Re: [HACKERS] Prep object creation hooks, and related sepgsql updates
Hi, Kohei KaiGai kai...@kaigai.gr.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(-) -- 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] Prep object creation hooks, and related sepgsql updates
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 USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us -- 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] Prep object creation hooks, and related sepgsql updates
2011/12/3 Robert Haas robertmh...@gmail.com: On Fri, Dec 2, 2011 at 6:52 AM, Kohei KaiGai kai...@kaigai.gr.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 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] Prep object creation hooks, and related sepgsql updates
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 kai...@kaigai.gr.jp:
2011/11/28 Dimitri Fontaine dimi...@2ndquadrant.fr:
Kohei KaiGai kai...@kaigai.gr.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 kai...@kaigai.gr.jp
--
KaiGai Kohei kai...@kaigai.gr.jp
pgsql-v9.2-sepgsql-create-permissions-part-1.database.patch
Description: Binary data
pgsql-v9.2-sepgsql-create-permissions-part-4.proc.patch
Description: Binary data
pgsql-v9.2-sepgsql-create-permissions-part-3.relation.patch
Description: Binary data
pgsql-v9.2-sepgsql-create-permissions-part-2.namespace.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
Re: [HACKERS] Prep object creation hooks, and related sepgsql updates
On Fri, Dec 2, 2011 at 6:52 AM, Kohei KaiGai kai...@kaigai.gr.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 -- 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] Prep object creation hooks, and related sepgsql updates
2011/11/28 Dimitri Fontaine dimi...@2ndquadrant.fr: Kohei KaiGai kai...@kaigai.gr.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 kai...@kaigai.gr.jp pgsql-v9.2-prep-creation-hook-part-1.v2.database.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
Re: [HACKERS] Prep object creation hooks, and related sepgsql updates
2011/11/27 Dimitri Fontaine dimi...@2ndquadrant.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 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] Prep object creation hooks, and related sepgsql updates
Kohei KaiGai kai...@kaigai.gr.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 -- 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] Prep object creation hooks, and related sepgsql updates
2011/11/28 Dimitri Fontaine dimi...@2ndquadrant.fr: Kohei KaiGai kai...@kaigai.gr.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 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] Prep object creation hooks, and related sepgsql updates
Kohei KaiGai kai...@kaigai.gr.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 cmdtrigger.ext.sql 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
Re: [HACKERS] Prep object creation hooks, and related sepgsql updates
2011/11/26 Dimitri Fontaine dimi...@2ndquadrant.fr: Kohei KaiGai kai...@kaigai.gr.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 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] Prep object creation hooks, and related sepgsql updates
Kohei KaiGai kai...@kaigai.gr.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 -- 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] Prep object creation hooks, and related sepgsql updates
2011/11/27 Dimitri Fontaine dimi...@2ndquadrant.fr: Kohei KaiGai kai...@kaigai.gr.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 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] Prep object creation hooks, and related sepgsql updates
Kohei KaiGai kai...@kaigai.gr.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 -- 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] Prep object creation hooks, and related sepgsql updates
Kohei KaiGai kai...@kaigai.gr.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
