Thread: [sepgsql 2/3] Add db_schema:search permission checks
This patch adds sepgsql support for permission checks equivalent to the existing SCHEMA USE privilege. This feature is constructed on new OAT_SCHEMA_SEARCH event type being invoked around pg_namespace_aclcheck(). So, its expected behavior also follows the behavior of existing permissions; unprivileged schema is ignored from the search path, or raise an error if object name is fully qualified. This patch needs src/backend/catalog/objectaccess.c is existing, so please apply this patch on top of this feature. https://commitfest.postgresql.org/action/patch_view?id=1003 Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
On 15 January 2013 20:28, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > This patch adds sepgsql support for permission checks equivalent > to the existing SCHEMA USE privilege. > > This feature is constructed on new OAT_SCHEMA_SEARCH event > type being invoked around pg_namespace_aclcheck(). Can you explain the exact detailed rationale behind this patch? Like URLs or other info that explains *why* we are doing this, what problems it causes if we don't, etc? Otherwise there is no reference point for a review. Other patch types like new features have syntax we can discuss and check, performance patches have measurements we can check. With this, it is just "we add some checks". No idea if that is all the places we need, or whether there is a better way of doing this, or whether anyone cares if we do this or not. (Same comment for patch 3/3) -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
2013/1/29 Simon Riggs <simon@2ndquadrant.com>:
> On 15 January 2013 20:28, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>
>> This patch adds sepgsql support for permission checks equivalent
>> to the existing SCHEMA USE privilege.
>>
>> This feature is constructed on new OAT_SCHEMA_SEARCH event
>> type being invoked around pg_namespace_aclcheck().
>
> Can you explain the exact detailed rationale behind this patch? Like
> URLs or other info that explains *why* we are doing this, what
> problems it causes if we don't, etc?
>
Sorry for this inconvenient. The second and third sepgsql patch try to
add permission checks on the places being not covered by sepgsql,
even though we defined related permissions; search of db_schema
and execute of db_procedure.
It is unavailable to control user's access towards these objects from
perspective of selinux policy, without these patches, even though
existing DAC mechanism well controls.
Right now, sepgsql applies no checks when users tried to lookup
an object name underlying a particular schema. It is inconvenient
to set up an environment that enforces per-domain namespace
according to the selinux basis, because current sepgsql ignores
searching schema, thus, it means all the schema is allowed to
search from viewpoint of selinux.
What we want to do is almost same as existing permission checks
are doing when users tried to search a particular schema, except
for its criteria to make access control decision.
Also, sepgsql applies no checks when users tries to execute
functions, right now. It makes unavailable to control execution of
functions from viewpoint of selinux, and here is no way selinux
to prevent to execute functions defined by other domains, or
others being not permitted.
Also, what we want to do is almost same as existing permission
checks, except for its criteria to make access control decision.
SELinux model requires client domain has db_schema:{search}
permission when it tries to search its underlying objects, and
client domain has db_procedure:{execute} permission when
it tries to execute the function and db_procedure:{entrypoint}
additionally if this execution performs as entrypoint of trusted-
procedures.
These are the problems behind of these patches.
In short, I'd like to give sepgsql configuration more flexibility
as existing DAC doing. That helps use cases of typical SaaS
applications that shares a database with multiple clients but
to be separated each other.
It is the motivation why I'd like to add these permissions here.
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
On 29 January 2013 13:30, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > It makes unavailable to control execution of > functions from viewpoint of selinux, and here is no way selinux > to prevent to execute functions defined by other domains, or > others being not permitted. > Also, what we want to do is almost same as existing permission > checks, except for its criteria to make access control decision. Do you have a roadmap of all the things this relates to? If selinux has a viewpoint, I'd like to be able to see a list of capabilities and then which ones are currently missing. I guess I'm looking for external assurance that someone somewhere needs this and that it fits into a complete overall plan of what we should do. Just like we are able to use SQLStandard as a guide as to what we need to implement, we would like something to refer back to. Does this have a request id, specification document page number or whatever? -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
2013/1/29 Simon Riggs <simon@2ndquadrant.com>: > On 29 January 2013 13:30, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > >> It makes unavailable to control execution of >> functions from viewpoint of selinux, and here is no way selinux >> to prevent to execute functions defined by other domains, or >> others being not permitted. >> Also, what we want to do is almost same as existing permission >> checks, except for its criteria to make access control decision. > > Do you have a roadmap of all the things this relates to? > > If selinux has a viewpoint, I'd like to be able to see a list of > capabilities and then which ones are currently missing. I guess I'm > looking for external assurance that someone somewhere needs this and > that it fits into a complete overall plan of what we should do. Just > like we are able to use SQLStandard as a guide as to what we need to > implement, we would like something to refer back to. Does this have a > request id, specification document page number or whatever? > I previously made several wiki pages for reference of permissions to be checked, but it needs maintenance works towards the latest state, such as newly added permissions. http://wiki.postgresql.org/wiki/SEPostgreSQL_References Even though selinuxproject.org hosts permission list, it is more rough than what I described at wiki.postgresql.org. http://www.selinuxproject.org/page/ObjectClassesPerms#Database_Object_Classes Unlike SQL standard, we have less resource to document its spec being validated by third persons. However, it is a reasonable solution to write up which permission shall be checked on which timing. Let me revise the above wikipage to show my overall plan. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On 29 January 2013 14:39, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > 2013/1/29 Simon Riggs <simon@2ndquadrant.com>: >> On 29 January 2013 13:30, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> >>> It makes unavailable to control execution of >>> functions from viewpoint of selinux, and here is no way selinux >>> to prevent to execute functions defined by other domains, or >>> others being not permitted. >>> Also, what we want to do is almost same as existing permission >>> checks, except for its criteria to make access control decision. >> >> Do you have a roadmap of all the things this relates to? >> >> If selinux has a viewpoint, I'd like to be able to see a list of >> capabilities and then which ones are currently missing. I guess I'm >> looking for external assurance that someone somewhere needs this and >> that it fits into a complete overall plan of what we should do. Just >> like we are able to use SQLStandard as a guide as to what we need to >> implement, we would like something to refer back to. Does this have a >> request id, specification document page number or whatever? >> > I previously made several wiki pages for reference of permissions > to be checked, but it needs maintenance works towards the latest > state, such as newly added permissions. > http://wiki.postgresql.org/wiki/SEPostgreSQL_References > > Even though selinuxproject.org hosts permission list, it is more > rough than what I described at wiki.postgresql.org. > http://www.selinuxproject.org/page/ObjectClassesPerms#Database_Object_Classes > > Unlike SQL standard, we have less resource to document its spec > being validated by third persons. However, it is a reasonable solution > to write up which permission shall be checked on which timing. > > Let me revise the above wikipage to show my overall plan. OK, that's looking like a good and useful set of info. What we need to do is to give the SELinux API a spec/version number (yes, the SELinux one) and then match what PostgreSQL implements against that, so we can say we are moving towards spec compliance with 1.0 and we have a list of unimplemented features... That puts this in a proper context, so we know what we are doing, why we are doing it and also when we've finished it. And also, how to know what future external changes will cause additional work. -- Simon Riggs http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 01/29/2013 10:10 PM, Simon Riggs wrote: > On 29 January 2013 13:30, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > >> It makes unavailable to control execution of >> functions from viewpoint of selinux, and here is no way selinux >> to prevent to execute functions defined by other domains, or >> others being not permitted. >> Also, what we want to do is almost same as existing permission >> checks, except for its criteria to make access control decision. > Do you have a roadmap of all the things this relates to? > > If selinux has a viewpoint, I'd like to be able to see a list of > capabilities and then which ones are currently missing. I guess I'm > looking for external assurance that someone somewhere needs this and > that it fits into a complete overall plan of what we should do. Just > like we are able to use SQLStandard as a guide as to what we need to > implement, we would like something to refer back to. Does this have a > request id, specification document page number or whatever? I think that would greatly assist people in understanding why these patches are neccessary, what real-world functionality they lead to, and what problems they solve. Some info on the wiki may be a good option. For example, if you were to say "these changes will help with multi-tenant PostgreSQL installations by <x>" then that will catch some people's eye. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Sorry for my late updates. I tried to update list of permissions that sepgsql expects, even though the description might be still a bit rough... https://wiki.postgresql.org/wiki/SEPostgreSQL_Permissions Set of permissions are defined for each object class that represents a particular database object. This list summarize all the defined permissions and introduction of the case when it shall be checked. Right now, the list of permissions are based on the latest selinux policy release at 20120725, but db_materialized_view class will be (probably) added in the future release somewhere in 2013. So, I added a short mention of this. My 2/3 and 3.3 patch try to add support "search" permission of db_schema class and "execute" permission of db_procedure class. It tries to implement relevant checks, but not supported yet. Does the permission list help to understand what does these patch try to tackle? Thanks, 2013/1/29 Simon Riggs <simon@2ndquadrant.com>: > On 29 January 2013 14:39, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> 2013/1/29 Simon Riggs <simon@2ndquadrant.com>: >>> On 29 January 2013 13:30, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >>> >>>> It makes unavailable to control execution of >>>> functions from viewpoint of selinux, and here is no way selinux >>>> to prevent to execute functions defined by other domains, or >>>> others being not permitted. >>>> Also, what we want to do is almost same as existing permission >>>> checks, except for its criteria to make access control decision. >>> >>> Do you have a roadmap of all the things this relates to? >>> >>> If selinux has a viewpoint, I'd like to be able to see a list of >>> capabilities and then which ones are currently missing. I guess I'm >>> looking for external assurance that someone somewhere needs this and >>> that it fits into a complete overall plan of what we should do. Just >>> like we are able to use SQLStandard as a guide as to what we need to >>> implement, we would like something to refer back to. Does this have a >>> request id, specification document page number or whatever? >>> >> I previously made several wiki pages for reference of permissions >> to be checked, but it needs maintenance works towards the latest >> state, such as newly added permissions. >> http://wiki.postgresql.org/wiki/SEPostgreSQL_References >> >> Even though selinuxproject.org hosts permission list, it is more >> rough than what I described at wiki.postgresql.org. >> http://www.selinuxproject.org/page/ObjectClassesPerms#Database_Object_Classes >> >> Unlike SQL standard, we have less resource to document its spec >> being validated by third persons. However, it is a reasonable solution >> to write up which permission shall be checked on which timing. >> >> Let me revise the above wikipage to show my overall plan. > > OK, that's looking like a good and useful set of info. > > What we need to do is to give the SELinux API a spec/version number > (yes, the SELinux one) and then match what PostgreSQL implements > against that, so we can say we are moving towards spec compliance with > 1.0 and we have a list of unimplemented features... > > That puts this in a proper context, so we know what we are doing, why > we are doing it and also when we've finished it. And also, how to know > what future external changes will cause additional work. > > -- > Simon Riggs http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services -- KaiGai Kohei <kaigai@kaigai.gr.jp>
On Tue, Jan 15, 2013 at 3:28 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > This patch adds sepgsql support for permission checks equivalent > to the existing SCHEMA USE privilege. > > This feature is constructed on new OAT_SCHEMA_SEARCH event > type being invoked around pg_namespace_aclcheck(). > So, its expected behavior also follows the behavior of existing > permissions; unprivileged schema is ignored from the search path, > or raise an error if object name is fully qualified. > > This patch needs src/backend/catalog/objectaccess.c is existing, > so please apply this patch on top of this feature. > https://commitfest.postgresql.org/action/patch_view?id=1003 KaiGai, Could you please rebase this patch? Thanks, -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2013/4/1 Robert Haas <robertmhaas@gmail.com>: > On Tue, Jan 15, 2013 at 3:28 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> This patch adds sepgsql support for permission checks equivalent >> to the existing SCHEMA USE privilege. >> >> This feature is constructed on new OAT_SCHEMA_SEARCH event >> type being invoked around pg_namespace_aclcheck(). >> So, its expected behavior also follows the behavior of existing >> permissions; unprivileged schema is ignored from the search path, >> or raise an error if object name is fully qualified. >> >> This patch needs src/backend/catalog/objectaccess.c is existing, >> so please apply this patch on top of this feature. >> https://commitfest.postgresql.org/action/patch_view?id=1003 > > KaiGai, > > Could you please rebase this patch? > OK, please check the attached ones. Both patches were rebased to the latest master branch, thus, once either of them got committed, another one has to be rebased later. Please also pay attention security policy module for regression test was also adjusted for these features. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
On Tue, Apr 2, 2013 at 2:22 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > OK, please check the attached ones. Thanks. I reviewed the schema-search patch and I think it looks reasonable, but shouldn't we be calling the event OAT_NAMESPACE_SEARCH rather than OAT_SCHEMA_SEARCH? And, similarly, ObjectAccessNamespaceSearch and InvokeNamespaceSearchHook? I think this terminology could be confusing to backend hackers who are used to seeing the term "namespace" internally when referring to schemas. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2013/4/3 Robert Haas <robertmhaas@gmail.com>: > On Tue, Apr 2, 2013 at 2:22 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> OK, please check the attached ones. > > Thanks. I reviewed the schema-search patch and I think it looks > reasonable, but shouldn't we be calling the event OAT_NAMESPACE_SEARCH > rather than OAT_SCHEMA_SEARCH? And, similarly, > ObjectAccessNamespaceSearch and InvokeNamespaceSearchHook? I think > this terminology could be confusing to backend hackers who are used to > seeing the term "namespace" internally when referring to schemas. > OK, I follow the manner of the terminology as we usually call it. The attached patch just replaced things you suggested. I'll also rebase the function-exec permission stuff later. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
On Thu, Apr 4, 2013 at 8:26 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > OK, I follow the manner of the terminology as we usually call it. > The attached patch just replaced things you suggested. Thanks, I have committed this, after making some changes to the comments and documentation. Please review the changes and let me know if you see any mistakes. BTW, if it were possible to set things up so that the test_sepgsql script could validate the version of the sepgsql-regtest policy installed, that would eliminate a certain category of errors. I notice also that the regression tests themselves seem to fail if you invoke the script as contrib/sepgsql/test_sepgsql rather than ./test_sepgsql, which suggests another possible pre-validation step. Thanks, -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2013/4/5 Robert Haas <robertmhaas@gmail.com>: > On Thu, Apr 4, 2013 at 8:26 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> OK, I follow the manner of the terminology as we usually call it. >> The attached patch just replaced things you suggested. > > Thanks, I have committed this, after making some changes to the > comments and documentation. Please review the changes and let me know > if you see any mistakes. > Thanks. I could find two obvious wording stuffs here, please see smaller one of the attached patches. I didn't fixup manner to use "XXX" in source code comments. Also, the attached function-execute-permission patch is a rebased version. I rethought its event name should be OAT_FUNCTION_EXECUTE, rather than OAT_FUNCTION_EXEC according to the manner without abbreviation. Other portion is same as previous ones. > BTW, if it were possible to set things up so that the test_sepgsql > script could validate the version of the sepgsql-regtest policy > installed, that would eliminate a certain category of errors. I > notice also that the regression tests themselves seem to fail if you > invoke the script as contrib/sepgsql/test_sepgsql rather than > ./test_sepgsql, which suggests another possible pre-validation step. > Please see the test-script-fixup patch. I added "cd `dirname $0`" on top of the script. It makes pg_regress to avoid this troubles. Probably, pg_regress was unavailable to read sql commands to run. A problem regarding to validation of sepgsql-regtest policy module is originated by semodule commands that takes root privilege to list up installed policy modules. So, I avoided to use this command in the test_sepgsql script. However, I have an idea that does not raise script fail even if "sudo semodule -l" returned an error, except for a case when it can run correctly and the policy version is not expected one. How about your opinion for this check? Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
On Mon, Apr 8, 2013 at 12:28 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > Thanks. I could find two obvious wording stuffs here, please see smaller > one of the attached patches. I didn't fixup manner to use "XXX" in source > code comments. Committed. > Also, the attached function-execute-permission patch is a rebased > version. I rethought its event name should be OAT_FUNCTION_EXECUTE, > rather than OAT_FUNCTION_EXEC according to the manner without > abbreviation. Other portion is same as previous ones. Great. This looks fine to me, committed. I also committed your getObjectIdentity patch, which caused some regression test output changes. I applied the necessary correction before committing, I think, but please check. >> BTW, if it were possible to set things up so that the test_sepgsql >> script could validate the version of the sepgsql-regtest policy >> installed, that would eliminate a certain category of errors. I >> notice also that the regression tests themselves seem to fail if you >> invoke the script as contrib/sepgsql/test_sepgsql rather than >> ./test_sepgsql, which suggests another possible pre-validation step. >> > Please see the test-script-fixup patch. > I added "cd `dirname $0`" on top of the script. It makes pg_regress to > avoid this troubles. Probably, pg_regress was unavailable to read > sql commands to run. > > A problem regarding to validation of sepgsql-regtest policy module > is originated by semodule commands that takes root privilege to > list up installed policy modules. So, I avoided to use this command > in the test_sepgsql script. > However, I have an idea that does not raise script fail even if "sudo > semodule -l" returned an error, except for a case when it can run > correctly and the policy version is not expected one. > How about your opinion for this check? Not sure that's too useful. And I don't like the idea of putting sudo commands in a test harness script. That seems too much like the sort of thing bad people do. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas escribió: > On Mon, Apr 8, 2013 at 12:28 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > > Also, the attached function-execute-permission patch is a rebased > > version. I rethought its event name should be OAT_FUNCTION_EXECUTE, > > rather than OAT_FUNCTION_EXEC according to the manner without > > abbreviation. Other portion is same as previous ones. > > Great. This looks fine to me, committed. I also committed your > getObjectIdentity patch, which caused some regression test output > changes. I applied the necessary correction before committing, I > think, but please check. I think the function-execute code path is still using getObjectDescription rather than identity. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Apr 12, 2013 at 10:42 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Robert Haas escribió: >> On Mon, Apr 8, 2013 at 12:28 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > >> > Also, the attached function-execute-permission patch is a rebased >> > version. I rethought its event name should be OAT_FUNCTION_EXECUTE, >> > rather than OAT_FUNCTION_EXEC according to the manner without >> > abbreviation. Other portion is same as previous ones. >> >> Great. This looks fine to me, committed. I also committed your >> getObjectIdentity patch, which caused some regression test output >> changes. I applied the necessary correction before committing, I >> think, but please check. > > I think the function-execute code path is still using > getObjectDescription rather than identity. Yeah, I think so. KaiGai, can you provide a follow-on patch to fix that? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
2013/4/12 Robert Haas <robertmhaas@gmail.com>: > On Fri, Apr 12, 2013 at 10:42 AM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: >> Robert Haas escribió: >>> On Mon, Apr 8, 2013 at 12:28 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: >> >>> > Also, the attached function-execute-permission patch is a rebased >>> > version. I rethought its event name should be OAT_FUNCTION_EXECUTE, >>> > rather than OAT_FUNCTION_EXEC according to the manner without >>> > abbreviation. Other portion is same as previous ones. >>> >>> Great. This looks fine to me, committed. I also committed your >>> getObjectIdentity patch, which caused some regression test output >>> changes. I applied the necessary correction before committing, I >>> think, but please check. >> >> I think the function-execute code path is still using >> getObjectDescription rather than identity. > > Yeah, I think so. KaiGai, can you provide a follow-on patch to fix that? > Yes, of course. The attached one replaces the getObjectDescription in sepgsql/proc.c, and relative changes in regression test. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
>> A problem regarding to validation of sepgsql-regtest policy module >> is originated by semodule commands that takes root privilege to >> list up installed policy modules. So, I avoided to use this command >> in the test_sepgsql script. >> However, I have an idea that does not raise script fail even if "sudo >> semodule -l" returned an error, except for a case when it can run >> correctly and the policy version is not expected one. >> How about your opinion for this check? > > Not sure that's too useful. And I don't like the idea of putting sudo > commands in a test harness script. That seems too much like the sort > of thing bad people do. > OK, I also doubt whether my idea make sense. The attached patch omitted the portion to check the version of sepgsql-regtest, and add some notice in the document instead. Also, it moves current directory to the contrib/sepgsql on top of the script, to avoid the problem when we run test_sepgsql on the directory except for contring/sepgsql. Thanks, -- KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachment
On Fri, Apr 12, 2013 at 2:44 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote: > Yes, of course. The attached one replaces the getObjectDescription in > sepgsql/proc.c, and relative changes in regression test. Thanks. Committed. I also committed the first two hunks of your cleanup patch but omitted the third one, which is not well-worded and seems like overkill anyway. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company