Re: Prep object creation hooks, and related sepgsql updates

Lists: pgsql-hackers
From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Prep object creation hooks, and related sepgsql updates
Date: 2011-11-15 11:04:01
Message-ID: CADyhKSWTt=N_SUpSaooYX11dTLW15NTtxHQUT+gE_ic70qoNwQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

These attached patches were getting rebased to the latest master and cosmetic
changes towrad upcoming commit-fest Nov.
Please apply these patches in order of Part-1, Part-2, Part-3 then Part-4.

We still don't have clear direction of the way to implement external permission
checks on object creation time. So, please consider these patches are on the
proof-of-concept stage; using prep-creation-hook to permission checks.

The reasons why existing post-creation-hook is not perfect to
implement permission
check on object creation time are:

(A) post-creation-hook is also invoked when we don't want to apply
permission checks.
For example, make_new_heap() invokes heap_create_with_catalog() to create
a temporary table for internal stuff as a part of CLUSTER, VACUUME FULL or some
of ALTER TABLE statements. However, it is not an option to deliver a
flag to show
whether this context needs permission checks, because it depends on
the knowledge
of individual security modules.

(B) system catalog does not contain all the necessary information for
permission checks.
Only pg_database hits this case. Existing DAC checks createdb
privilege and ownership
of the source database (if not templace), however, pg_database does
not contain oid of
the source database, so it is unavailable to apply equivalent
permission checks on the
external security modules.

Thus, the current solution is put new hooks (prep-creation) around
existing permission
checks with arguments being used to existing DAC checks commonly.
Since the argument has a pointer of the private opaque to be delivered
to post-creation
hooks. In sepgsql case, it is used to inform post-creation hook
security label to be
assigned on the new object being constructed.

Even though this approach enables to solve the issue, I'm not sure
whether we should
have prep-creation hooks for each object types in an automatic manner.
We currently have 30 of post-creation hooks for each object types,
however, most of
them does not hit the case of above either (A) or (B).
I guess pg_database, pg_class and pg_trigger (it takes "isInternal"
flag to avoid
nonsense permission checks) are the only case that hits post-creation hook is
not enough to implement correct permission checks.
So, I'm not sure to add prep-creation hook on rest of (90% of ) object
types, although
here is few strong reason to split creation-hook into two parts.

Thanks,

2011/11/12 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2011/11/8 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Mon, Nov 7, 2011 at 12:20 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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.
>>
> I tried to strip the previous patch into small portion per object types.
> Part-1 for database, Part-2 for schema, Part-3 for relations, and
> Part-4 for functions.
>
> The basic idea is the prep-creation hook informs the sepgsql module
> properties of the new object being constructed. According to the
> previous discussion, all the arguments are commonly used to existing
> DAC checks also. Then, this hook allows to write back its private
> opaque to be delivered to the post-creation hook; likely used to deliver
> security label of the new object to be assigned.
>
> However, I become unsure whether it is a good idea to put prep-creation
> hook on all the object, because it takes many boring interface changes
> to deliver private datum, and we're probably able to implement similar
> stuff with post-creation hook except for a few object types.
>
> I guess the following (A) and (B) are the only case that needs prep-
> creation hooks for permission checks. Elsewhere, sepgsql will be
> able to make its decision based on the entry of system catalogs
> on post-creation hook.
>
> (A) In the case when we want to apply checks based on information
> that is not contained within system catalogs.
> E.g, Oid of source database on CREATE DATABASE. Existing DAC
> checks ownership of the database, if not template.
>
> (B) In the case when we want to distinguish code path between user's
> query and system internal stuff.
> E.g, heap_create_with_catalog() is also called by make_new_heap()
> as a part of ALTER TABLE, but it is quite internal stuff, so not suitable
> to apply permission checks here.
>
> It seems to me, using post-creation hooks makes the patch mode simple
> to implement permission checks; except for above two object types.
> So, I'd like to adopt approach to put prep-creation hooks on limited number
> of object types, not symmetric with post-creation hook.
> How about your opinion about this?
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Attachment Content-Type Size
pgsql-v9.2-prep-creation-hook-part-1.v1.database.patch application/octet-stream 14.1 KB
pgsql-v9.2-prep-creation-hook-part-4.v1.procedure.patch application/octet-stream 17.2 KB
pgsql-v9.2-prep-creation-hook-part-3.v1.relation.patch application/octet-stream 22.7 KB
pgsql-v9.2-prep-creation-hook-part-2.v1.schema.patch application/octet-stream 11.1 KB

From: Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr>
To: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-11-26 19:36:40
Message-ID: m2ty5q7k1j.fsf@2ndQuadrant.fr
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> We still don't have clear direction of the way to implement external permission
> checks on object creation time. So, please consider these patches are on the
> proof-of-concept stage; using prep-creation-hook to permission checks.

I wonder if you could implement that as an extension given the command
trigger patch finds its way in. What do you think?

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-11-27 19:55:18
Message-ID: CADyhKSXH_sdn11vh8vaz4f1s91b+QTM3ZPDZieHLZuUnmHTiXg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

2011/11/26 Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> We still don't have clear direction of the way to implement external permission
>> checks on object creation time. So, please consider these patches are on the
>> proof-of-concept stage; using prep-creation-hook to permission checks.
>
> I wonder if you could implement that as an extension given the command
> trigger patch finds its way in.  What do you think?
>
Unfortunately, it does not solve my point.

My proposition allows an extension to deliver an opaque value being set up
at the prep-creation hook into post-creation hook. It shall be used to deliver
a security label to be assigned on the new object, however, it is unavailable
to assign on prep-creation phase, because its object-id is not fixed yet.
(It is not an option to ask operating system a default security label of the
new object twice, because security policy may be reloaded between prep-
and post-.)

It is also reason why I mentioned about an idea that put prep-creation hook
on a limited number of object classes only. It requires us code modification
to maintain an opaque private between prep- and post- hooks.

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


From: Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr>
To: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
Cc: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-11-27 20:24:39
Message-ID: m2ehwt5n5k.fsf@2ndQuadrant.fr
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> I wonder if you could implement that as an extension given the command
>> trigger patch finds its way in.  What do you think?
>>
> Unfortunately, it does not solve my point.

[...]

> It is also reason why I mentioned about an idea that put prep-creation hook
> on a limited number of object classes only. It requires us code modification
> to maintain an opaque private between prep- and post- hooks.

In my current proposal for command triggers, the trigger procedure is
given schemaname and objectname as separate arguments. It seems to me
easy enough to use that as a key to some data structure where the value
is any opaque data you need, and that you maintain in your extension
triggers code. You can write them in C.

I don't think schemaname+objectname fails to be unique, so I don't think
you need another kind of Oid in BEFORE creation triggers here.

--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-11-27 20:51:09
Message-ID: CADyhKSUTyjf1d=Qsdub0Aii4iFW9ZJS_FfHFqDJJMRMAXGG_GA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

2011/11/27 Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>>> I wonder if you could implement that as an extension given the command
>>> trigger patch finds its way in.  What do you think?
>>>
>> Unfortunately, it does not solve my point.
>
> [...]
>
>> It is also reason why I mentioned about an idea that put prep-creation hook
>> on a limited number of object classes only. It requires us code modification
>> to maintain an opaque private between prep- and post- hooks.
>
> In my current proposal for command triggers, the trigger procedure is
> given schemaname and objectname as separate arguments.  It seems to me
> easy enough to use that as a key to some data structure where the value
> is any opaque data you need, and that you maintain in your extension
> triggers code. You can write them in C.
>
Sorry, it does not cover all the code paths that I want to apply permission
checks around creation of new tables.

The existing DAC checks permission on creation of new tables at
DefineRelation() and OpenIntoRel(), and sepgsql also wants to follow
this manner.
However, OpenIntoRel() does not go through ProcessUtility, so it seems
to me the command trigger is not invoked in this case.

And, it seems to me the current proposition of the command trigger
does not support to fire triggers on creation of databases, although
permission checks requires Oid of source database that is not also
appeared in pg_database catalog.

> I don't think schemaname+objectname fails to be unique, so I don't think
> you need another kind of Oid in BEFORE creation triggers here.
>
The pg_seclabel and pg_shseclabel needs OID to assign a security label
on a particular database object, so label provider (sepgsql) must know
Oid of the target object on assignment time.

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


From: Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr>
To: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
Cc: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-11-27 21:52:26
Message-ID: m2wral44it.fsf@2ndQuadrant.fr
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> Sorry, it does not cover all the code paths that I want to apply permission
> checks around creation of new tables.
>
> The existing DAC checks permission on creation of new tables at
> DefineRelation() and OpenIntoRel(), and sepgsql also wants to follow
> this manner.
> However, OpenIntoRel() does not go through ProcessUtility, so it seems
> to me the command trigger is not invoked in this case.

we have the same problem in the command trigger patch, we will need to
add specific calls to its functions from other code path than just
ProcessUtility.

> And, it seems to me the current proposition of the command trigger
> does not support to fire triggers on creation of databases, although
> permission checks requires Oid of source database that is not also
> appeared in pg_database catalog.

I have to have a look at what forbids us to add support for the create
database command here. It seems to be just another branch of the switch
in standard_ProcessUtility().

>> I don't think schemaname+objectname fails to be unique, so I don't think
>> you need another kind of Oid in BEFORE creation triggers here.
>>
> The pg_seclabel and pg_shseclabel needs OID to assign a security label
> on a particular database object, so label provider (sepgsql) must know
> Oid of the target object on assignment time.

Yes, and you need to refer to things you did in the BEFORE trigger from
the AFTER trigger, I'm just offering you a way to do that. Then if you
need the Oid in the AFTER trigger, of course you have it.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-11-28 14:16:23
Message-ID: CADyhKSV6zdSKoH19dYaaE-t_B3wSqCu1Km_tAbJGNn62B-+vTg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

2011/11/27 Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>:
>> And, it seems to me the current proposition of the command trigger
>> does not support to fire triggers on creation of databases, although
>> permission checks requires Oid of source database that is not also
>> appeared in pg_database catalog.
>
> I have to have a look at what forbids us to add support for the create
> database command here.  It seems to be just another branch of the switch
> in standard_ProcessUtility().
>
I'm not sure what arguments shall be provided to the guest of command
triggers. And, I'd like to mention about its specification should not depend
on details of particular modules; according to the previous discussion.

After the long discussion, one concensus is that prep-creation hook shall
be deployed around existing DAC checks and it takes arguments that are
also referenced at the existing DAC; such as oid of source database on
creation of databases.
I also checked security model between DAC and MAC, and I concluded
most of them takes common information to make its decision.

It is a hard to answer question whether we can implement sepgsql on the
upcoming command trigger feature; without information about where its
hooks shall be deployed and what arguments are provided. :-(

>>> I don't think schemaname+objectname fails to be unique, so I don't think
>>> you need another kind of Oid in BEFORE creation triggers here.
>>>
>> The pg_seclabel and pg_shseclabel needs OID to assign a security label
>> on a particular database object, so label provider (sepgsql) must know
>> Oid of the target object on assignment time.
>
> Yes, and you need to refer to things you did in the BEFORE trigger from
> the AFTER trigger, I'm just offering you a way to do that.  Then if you
> need the Oid in the AFTER trigger, of course you have it.
>
How does it inherit an opaque private initialized at BEFORE trigger to
AFTER trigger? I checked your patch, however, it seems to me it does
not have a mechanism to deliver something between BEFORE and AFTER.

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


From: Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr>
To: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
Cc: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-11-28 14:24:54
Message-ID: 87zkfg72a1.fsf@hi-media-techno.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> How does it inherit an opaque private initialized at BEFORE trigger to
> AFTER trigger? I checked your patch, however, it seems to me it does
> not have a mechanism to deliver something between BEFORE and AFTER.

Right, there's no such facility provided in there. But it seems to me
that your extension could attach some shared memory or use other
mechanisms to handle that on its own?

I'm not trying to force you into using command triggers, just to
determine if that's a road we are able to take.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-11-28 17:00:45
Message-ID: CADyhKSVCqurJdOOOztkPa_cyDqXDieJBBk_9yKjrSNabLSW+1g@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

2011/11/28 Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> How does it inherit an opaque private initialized at BEFORE trigger to
>> AFTER trigger? I checked your patch, however, it seems to me it does
>> not have a mechanism to deliver something between BEFORE and AFTER.
>
> Right, there's no such facility provided in there.  But it seems to me
> that your extension could attach some shared memory or use other
> mechanisms to handle that on its own?
>
Hmm. If extension side manage the contextual information by itself, it seems
to me feasible, although it is a bit bother.
I found up a similar idea that acquires control on ProcessUtility_hook and
save necessary contextual information on auto variable then kicks the
original ProcessUtility_hook, then it reference the contextual information
from object_access_hook.

For example, we don't want to apply permission checks on new relations
constructed with make_new_heap. It shall be invoked when CLUSTER,
VACUUM or ALTER TABLE, so we can skip permission checks when
the saved command tag indicates these commands are currently running.

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


From: Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr>
To: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
Cc: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-11-28 20:28:04
Message-ID: m2fwh8uh4b.fsf@2ndQuadrant.fr
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> I found up a similar idea that acquires control on ProcessUtility_hook and
> save necessary contextual information on auto variable then kicks the
> original ProcessUtility_hook, then it reference the contextual information
> from object_access_hook.

In this case that would be an INSTEAD OF trigger, from which you can
call the original command with EXECUTE. You just have to protect
yourself against infinite recursion, but that's doable. See attached
example.

> For example, we don't want to apply permission checks on new relations
> constructed with make_new_heap. It shall be invoked when CLUSTER,
> VACUUM or ALTER TABLE, so we can skip permission checks when
> the saved command tag indicates these commands are currently running.

CREATE TRIGGER se_permission_checks
INSTEAD OF COMMAND ALTER TABLE
EXECUTE PROCEDURE se_permission_checks_alter_table();

In this INSTEAD OF trigger, protect against recursion, EXECUTE the
original ALTER TABLE statement which is given to you as a parameter,
enable the command trigger again.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Attachment Content-Type Size
cmdtrigger.ext.sql application/octet-stream 1.2 KB

From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-11-29 16:42:47
Message-ID: CADyhKSVj5TzyPOiwH+RUqDso6ocxti9U+8uu0vmC0uZTEJEAgw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

2011/11/28 Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> I found up a similar idea that acquires control on ProcessUtility_hook and
>> save necessary contextual information on auto variable then kicks the
>> original ProcessUtility_hook, then it reference the contextual information
>> from object_access_hook.
>
> In this case that would be an INSTEAD OF trigger, from which you can
> call the original command with EXECUTE. You just have to protect
> yourself against infinite recursion, but that's doable. See attached
> example.
>
Hmm... In my case, it does not need to depend on the command trigger.
Let's see the attached patch; that hooks ProcessUtility_hook by
sepgsql_utility_command, then it saves contextual information on auto
variables.

The object_access_hook with OAT_POST_CREATE shall be invoked
from createdb() that was also called by standard_ProcessUtility.
In this context, sepgsql_database_post_create can reference
a property that is not contained withint pg_database catalog
(name of the source database).

At least, it may be able to solve my issues on hooks around object
creation time.

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

Attachment Content-Type Size
pgsql-v9.2-prep-creation-hook-part-1.v2.database.patch application/octet-stream 10.5 KB

From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-12-02 11:52:41
Message-ID: CADyhKSWr8Mt_rzu_dAN63Cnyx5G8yt=BjUmeL_p_7ZpUD_gM8g@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

I tried to implement remaining portion of the object creation permission patches
using this approach; that temporary saves contextual information using existing
ProcessUtility hook and ExecutorStart hook.

It likely works fine towards my first problem; system catalog entry
does not have
all the information that requires to make access control decision. In
the case of
pg_database catalog, it does not inform us which database was its source.

Also it maybe works towards my second problem; some of code paths internally
used invokes object-access-hook with OAT_POST_CREATE, so entension is
unavailable to decide whether permission checks should be applied, or not.
In the case of pg_class, heap_create_with_catalog() is invoked by
make_new_heap(),
not only DefineRelation() and OpenIntoRel().
So, this patch checks which statement eventually calls these routines to decide
necessity of permission checks.

All I did is a simple hack on ProcessUtility hook and ExecutorStart hook, then
post-creation-hook references the saved contextual information, as follows.

sepgsql_utility_command(...)
{
sepgsql_context_info_t saved_context_info = sepgsql_context_info;

PG_TRY()
{
sepgsql_context_info.cmdtype = nodeTag(parsetree);
:
if (next_ProcessUtility_hook)
(*next_ProcessUtility_hook) (....)
else
standard_ProcessUtility(....)
}
PG_CATCH();
{
sepgsql_context_info = saved_context_info;
PG_RE_THROW();
}
PG_END_TRY();
sepgsql_context_info = saved_context_info;
}

Then,

sepgsql_relation_post_create(....)
{
:
/*
* Some internally used code paths call heap_create_with_catalog(), then
* it launches this hook, even though it does not need permission check
* on creation of relation. So, we skip these cases.
*/
switch (sepgsql_context_info.cmdtype)
{
case T_CreateStmt:
case T_ViewStmt:
case T_CreateSeqStmt:
case T_CompositeTypeStmt:
case T_CreateForeignTableStmt:
case T_SelectStmt:
break;
default:
/* internal calls */
return;
}
:
}

At least, it is working. However, it is not a perfect solution to the
future updates
of code paths in the core.

Thanks,

2011/11/29 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2011/11/28 Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>:
>> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>>> I found up a similar idea that acquires control on ProcessUtility_hook and
>>> save necessary contextual information on auto variable then kicks the
>>> original ProcessUtility_hook, then it reference the contextual information
>>> from object_access_hook.
>>
>> In this case that would be an INSTEAD OF trigger, from which you can
>> call the original command with EXECUTE. You just have to protect
>> yourself against infinite recursion, but that's doable. See attached
>> example.
>>
> Hmm... In my case, it does not need to depend on the command trigger.
> Let's see the attached patch; that hooks ProcessUtility_hook by
> sepgsql_utility_command, then it saves contextual information on auto
> variables.
>
> The object_access_hook with OAT_POST_CREATE shall be invoked
> from createdb() that was also called by standard_ProcessUtility.
> In this context, sepgsql_database_post_create can reference
> a property that is not contained withint pg_database catalog
> (name of the source database).
>
> At least, it may be able to solve my issues on hooks around object
> creation time.
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

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

Attachment Content-Type Size
pgsql-v9.2-sepgsql-create-permissions-part-1.database.patch application/octet-stream 10.1 KB
pgsql-v9.2-sepgsql-create-permissions-part-4.proc.patch application/octet-stream 5.7 KB
pgsql-v9.2-sepgsql-create-permissions-part-3.relation.patch application/octet-stream 14.4 KB
pgsql-v9.2-sepgsql-create-permissions-part-2.namespace.patch application/octet-stream 4.1 KB

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
Cc: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-12-03 04:55:38
Message-ID: CA+TgmoZPLsGJ8icZZnzHAo81Emtk7+tAZ+UrYSLJz4=HKST1Cw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Dec 2, 2011 at 6:52 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> At least, it is working. However, it is not a perfect solution to the
> future updates
> of code paths in the core.

Hmm. So, do you want this committed? If so, I think the major thing
it lacks is documentation.

I can't help noticing that this amounts, altogether, to less than 600
lines of code. I am not sure what your hesitation in taking this
approach is. Certainly, there are things not to like in here, but
I've seen a lot worse, and you can always refine it later. For a
first cut, why not? Even if you had the absolutely perfect hooks in
core, how much would it save compared to what's here now? How
different would your ideal implementation be from what you've done
here?

As regards future updates of code paths in core, nothing in here looks
terribly likely to get broken; or at least if it does then I think
quite a lot of other things will get broken, too. Anything we do has
some maintenance burden, and this doesn't look particularly bad to me.

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


From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-12-03 08:43:27
Message-ID: CADyhKSWxyqkJhEpa7AGAEJQojR=F5Lj=RqLF3n3LyNGV1HZLHA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

2011/12/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Dec 2, 2011 at 6:52 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> At least, it is working. However, it is not a perfect solution to the
>> future updates
>> of code paths in the core.
>
> Hmm.  So, do you want this committed?  If so, I think the major thing
> it lacks is documentation.
>
> I can't help noticing that this amounts, altogether, to less than 600
> lines of code.  I am not sure what your hesitation in taking this
> approach is.  Certainly, there are things not to like in here, but
> I've seen a lot worse, and you can always refine it later.  For a
> first cut, why not?    Even if you had the absolutely perfect hooks in
> core, how much would it save compared to what's here now?  How
> different would your ideal implementation be from what you've done
> here?
>
You are likely right. Even if the hook provides sepgsql enough
contextual information, it might means maintenance burden being
moved to the core from sepgsql, as we discussed before.
OK, I'd like to go with this approach. I'll try to update documentation
stuff and regression test cases, so please wait for a few days.

Thanks,

> As regards future updates of code paths in core, nothing in here looks
> terribly likely to get broken; or at least if it does then I think
> quite a lot of other things will get broken, too.  Anything we do has
> some maintenance burden, and this doesn't look particularly bad to me.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

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


From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-12-05 13:54:09
Message-ID: CADyhKSUwK1TNioAYwQeEnHEFy6zarRAAUn34ea8A4UQ5jPu-HQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

The attached patches are revised ones.
I added explanations of DDL permissions on creation time added by these patches,
and added a few regression test cases.

Thanks,

2011/12/3 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2011/12/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Fri, Dec 2, 2011 at 6:52 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> At least, it is working. However, it is not a perfect solution to the
>>> future updates
>>> of code paths in the core.
>>
>> Hmm.  So, do you want this committed?  If so, I think the major thing
>> it lacks is documentation.
>>
>> I can't help noticing that this amounts, altogether, to less than 600
>> lines of code.  I am not sure what your hesitation in taking this
>> approach is.  Certainly, there are things not to like in here, but
>> I've seen a lot worse, and you can always refine it later.  For a
>> first cut, why not?    Even if you had the absolutely perfect hooks in
>> core, how much would it save compared to what's here now?  How
>> different would your ideal implementation be from what you've done
>> here?
>>
> You are likely right. Even if the hook provides sepgsql enough
> contextual information, it might means maintenance burden being
> moved to the core from sepgsql, as we discussed before.
> OK, I'd like to go with this approach. I'll try to update documentation
> stuff and regression test cases, so please wait for a few days.
>
> Thanks,
>
>> As regards future updates of code paths in core, nothing in here looks
>> terribly likely to get broken; or at least if it does then I think
>> quite a lot of other things will get broken, too.  Anything we do has
>> some maintenance burden, and this doesn't look particularly bad to me.
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>
>
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

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

Attachment Content-Type Size
pgsql-v9.2-sepgsql-create-permissions-part-4.v2.proc.patch application/octet-stream 5.8 KB
pgsql-v9.2-sepgsql-create-permissions-part-2.v2.schema.patch application/octet-stream 4.1 KB
pgsql-v9.2-sepgsql-create-permissions-part-3.v2.relation.patch application/octet-stream 17.3 KB
pgsql-v9.2-sepgsql-create-permissions-part-1.v2.database.patch application/octet-stream 13.1 KB

From: Dimitri Fontaine <dimitri(at)2ndQuadrant(dot)fr>
To: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, PgHacker <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-12-16 16:58:59
Message-ID: 8762hgmp0s.fsf@hi-media-techno.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hi,

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> The attached patches are revised ones.
> I added explanations of DDL permissions on creation time added by these patches,
> and added a few regression test cases.

The whole patches are now against contrib/sepgsql, which seems to me to
be a good news, but means I'm not skilled to help review further. I'm
unsure about marking that as “ready for commiter” but I'm definitely
done myself.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

As seen here, contrib only patch now:

[2. application/octet-stream; pgsql-v9.2-sepgsql-create-permissions-part-4.v2.proc.patch]...

contrib/sepgsql/expected/create.out | 13 +++++++-
contrib/sepgsql/proc.c | 55 +++++++++++++++++++++++++++++++----
contrib/sepgsql/sql/create.sql | 7 ++++
3 files changed, 68 insertions(+), 7 deletions(-)

[3. application/octet-stream; pgsql-v9.2-sepgsql-create-permissions-part-2.v2.schema.patch]...

contrib/sepgsql/expected/create.out | 4 +++
contrib/sepgsql/schema.c | 50 ++++++++++++++++++++++++++++++++---
contrib/sepgsql/sql/create.sql | 6 ++++
3 files changed, 56 insertions(+), 4 deletions(-)

[4. application/octet-stream; pgsql-v9.2-sepgsql-create-permissions-part-3.v2.relation.patch]...

contrib/sepgsql/expected/create.out | 46 +++++++++++
contrib/sepgsql/hooks.c | 70 +++++++++++++++++-
contrib/sepgsql/relation.c | 144 +++++++++++++++++++++++++++++------
contrib/sepgsql/sql/create.sql | 18 +++++
4 files changed, 254 insertions(+), 24 deletions(-)

[5. application/octet-stream; pgsql-v9.2-sepgsql-create-permissions-part-1.v2.database.patch]...

contrib/sepgsql/database.c | 91 ++++++++++++++++++++++----
contrib/sepgsql/expected/create.out | 19 ++++++
contrib/sepgsql/hooks.c | 122 ++++++++++++++++++++++++----------
contrib/sepgsql/sepgsql.h | 3 +-
contrib/sepgsql/sql/create.sql | 15 ++++
contrib/sepgsql/test_sepgsql | 2 +-
doc/src/sgml/sepgsql.sgml | 30 ++++++++-
7 files changed, 231 insertions(+), 51 deletions(-)


From: Greg Smith <greg(at)2ndQuadrant(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Prep object creation hooks, and related sepgsql updates
Date: 2011-12-16 17:55:05
Message-ID: 4EEB85F9.70701@2ndQuadrant.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 12/16/2011 11:58 AM, Dimitri Fontaine wrote:
> The whole patches are now against contrib/sepgsql, which seems to me to
> be a good news, but means I'm not skilled to help review further. I'm
> unsure about marking that as “ready for commiter” but I'm definitely
> done myself.
>

Robert already took a brief look at this upthread and suggested it
seemed in the right direction if issues like the docs were sorted out.
That sounds like "ready for committer" going in his direction to me,
particularly since there's not a lot of other people who are familiar
with the sepgsql side; relabeling it in the CF app accordingly. Note
that these were labeled as "proof-of-concept" in the original
submission, so a commit might not be the right next step even if they
look good so far.

--
Greg Smith 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us