Proposal of SE-PostgreSQL patches [try#2]
in [PgSql]
|
Prev: Hash index build patch has *worse* performance at small table sizes
Next: Dept of ugly hacks: eliminating padding space in system indexes
From: KaiGai Kohei on 23 Jun 2008 04:23 Hi, The following patch set is our second proposals of SE-PostgreSQL. It contains many of fixes and improvements from the previous version. Please add them a reviwing queue of the next commit fest. Thanks, List of Patches =============== [1/4] Core facilities of PGACE/SE-PostgreSQL http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r914.patch [2/4] "--enable-selinux" option of pg_dump/pg_dumpall http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r914.patch [3/4] Default security policy for SE-PostgreSQL http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r914.patch [4/4] Documentation updates http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r914.patch We can provide a quick overview for SE-PostgreSQL at: http://code.google.com/p/sepgsql/wiki/WhatIsSEPostgreSQL http://sepgsql.googlecode.com/files/PGCON20080523.pdf Compile and Installation ======================== The following items are requirements of SE-PostgreSQL. - Fedora 8 or later system - SELinux is enabled, and working - kernel-2.6.23 or later - selinux-policy and selinux-policy-devel v3.0.8 or later - libselinux, policycoreutils, checkpolicy The followings are step by step installation. $ cvs -z3 -d :pserver:anoncvs(a)anoncvs.postgresql.org:/projects/cvsroot \ export -r HEAD -d pgsql $ cd pgsql $ patch -p1 < sepostgresql-sepgsql-8.4devel-3-r914.patch $ patch -p1 < sepostgresql-pg_dump-8.4devel-3-r914.patch $ patch -p1 < sepostgresql-policy-8.4devel-3-r914.patch $ patch -p1 < sepostgresql-docs-8.4devel-3-r914.patch $ ./configure --enable-selinux $ make $ make -C ./contrib/sepgsql_policy $ su # /usr/sbin/semodule -i ./contrib/sepgsql_policy/sepostgresql.pp ... [1] # make install # /sbin/restorecon -R /usr/local/pgsql $ mkdir -p $PGDATA $ chcon -t postgresql_db_t -R $PGDATA $ initdb $ pg_ctl start [1] If "selinux-policy-3.4.2" or later is installed on your system, install "sepostgresql-devel.pp" instead. In this version, most of SE-PostgreSQL's policy are got mainlined. Updates from the previous version ================================= o A new type of "security_label" has gone In the previous one, "security_context" system column is declared as security_label type. This type had its input handler, and it translated a given text representation into an internal Oid value with looking up pg_security system catalog. If it's not found, the input handler inserts a new entry automatically. The following query can show the reason why this design is problematic. SELECT 'system_u:object_r:sepgsql_db_t'::security_label; This query seems to us read-only, but it has a possibility to insert a new tuple into pg_security implicitly. In this version, "security_context" system column is re-defined as a TEXT type attribute, and a given text representation is translated into internal identifier (Oid) just before insert or update a tuple. This design change enables to make sure pg_security is not modified in read-only queries. o Query modification has gone. In the previous one, SE-PostgreSQL modified WHERE/JOIN ON clause to apply tuple-level access controls, but its implementation is a bit complicated. In addition, this design had a possibility to conflict with a new MS patent. Now we put a hook on ExecScan to apply tuple-level access controls. It enables to reduce code complexity and avoid patent conflicts. o Scanning with SnapshotSelf has gone. In the previous one, we had to scan some system catalogs with SnapshotSelf mode at three points (pg_class, pg_largeobject and pg_security). * When we defines a relation on heap_create_with_catalog(), a tuple of pg_class and several tuples of pg_attribute are inserted within same command id. A tuple of pg_class has to be refered just before inserting tuples of pg_attribute, because a new column inherits the security context of its parent relation in the default. But we cannot find it because these are inserted within same command id and SnapshotNow scanning mode ignores these tuples. We cannot invoke CommandIdIncrement() here, because it finally checks integrity of relation cache, but the relation is not constructed yet. We can apply two option here. One is preserving the security context of parent table and applying it later without looking up pg_class. The other is to insert a temporary entry into SysCache until it is invalidated. The later approach can also be applied on the next situation, so we now put InsertSysCache() withing heap_create_with_catalog() to refer the new tuple before next CommandIdIncrement() which is invoked just after hecp_create_with_catalog(). * When a user gives a security context in text representation, it is translated into an internal identifier which indicates the oid of pg_security system catalog. If it was not found, PGACE/SE-PostgreSQL inserts a new tuple and applies its oid as an internal identifier. If a same new security context is given within same currentCommandId twice or more, it tries to insert a new tuple into pg_security twice or more. However, it violates uniqueness constraint at oid of pg_security. Thus, we had to look up pg_security with SnapshotSelf scanning mode as a fallback when SearchSysCache() returns invalid tuple. But we can apply same approach here. So, InsertSysCache() is invoked to keep a newly inserted security context until next CommandIdIncrement(). * When a user inserts or deletes a tuple within pg_largeobject directly, it can also means create a new larageobject, or drop ones. In SE-PostgreSQL model, it requires 'create' or 'drop' permission, so we had to check whether the tuple is the first/last one, or not. In this case, we assumes inserting a tuple into pg_largeobject directly has a possibility to create new largeobject, and 'create' permission should be always evaluated, not only 'write'. This assumption kills to scan pg_largeobject for each insertion/deletion. If requests come from lowrite() or lo_create(), we can distinguish its meanings, so proper permissions are applied in the most cases. I guess the InsertSysCache() will be an arguable interface, but it can resolve our problem with minimum impact and utilizing existing facilities, so it is better than any other solutuions. o A new guc parameter to enable/disable SE-PostgreSQL It can take four options, as follows: sepostgresql = [ default | enforcing | permissive | disabled ] - default: always follows kernel setting (default) - enforcing: works in enforcing mode (MAC and audit enabled). - permissive: works in permissive mode (audit log only). - disabled: disables SE-PostgreSQL feature. o PGACE hooks are inlined The contains of src/backend/security/pgaceHooks.c are moved to src/include/security/pgace.h and inlined. It enables to reduce cost to invoke empty function when this feature is disabled. o Generic writable system column SystemAttributeIsWritable() can abstract what system attribute is writable. (Currently, the security system catalog is the only one writable.) If it returns true on the target of INSERT, UPDATE or SELECT INTO, these TargetEntries are marked as junk, and we can fetch its value on ExecutorRun() separated from any other regular attribute. o early security design In the previous one, we stores a relationship between security id and text representation on bootstraping mode, because pg_security system catalog is not constructed yet in this time. The current version holds them in-memory cache and writes out on the tail of the bootstraping process. o Documentation updates The doc/src/sgml/security.sgml gives us a short description of SE-PostgreSQL and PGACE security framework. In addition, we added source code comments for most of functions, including PGACE hooks. o Miscellaneous updates * Two separated patches (pgace and sepgsql) are integrated into one. * Copyrights are changed to "PostgreSQL Global Development Group". * Several PGACE hooks are added, redefined and removed. * Now, we can run regression test without any problem, except for two tests which can be explained reasonably. * SELinux state monitoring process is implemented using an existing facilities provided by postmaster.c. * Coding styles are fixed. * A definition of LWlock is moved to storage/lwlock.h * Definitions of SEvalItemXXXX are moved to nodes/nodes.h * Compiler warnings come from SE-PostgreSQL are killed. * Some error codes are added within 'SE' class, and elog()s which can report user facing messages are replaced by ereport(). * Shell function is removed from genbki.sh. * Default security context of files consider --prefix setting in configure script. Regression Tests ================ Now we remain two test fails, but these can be explained reasonably. The first fail (create_function_1) means that SE-PostgreSQL detects a client attempt to load an invalid file before core PostgreSQL doing, and generates its error message. The later one (sanity_check) means the regression test can detect an increation of system catalogs correctly. (*) Turn on "sepgsql_regression_test_mode" boolean of SELinux before regression test. It enables you to load shared library modules installed under user's home directory. # /usr/sbin/setsebool sepgsql_regression_test_mode on $ make check : (snip) : ======================== 2 of 115 tests failed. ======================== [kaigai(a)saba pgsql]$ less src/test/regress/regression.diffs *** ./expected/create_function_1.out Fri Jun 20 14:55:12 2008 --- ./results/create_function_1.out Fri Jun 20 14:55:28 2008 *************** *** 72,78 **** ERROR: only one AS item needed for language "sql" CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C AS 'nosuchfile'; ! ERROR: could not access file "nosuchfile": No such file or directory CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C AS '/home/kaigai/tmp/e/pgsql/src/test/regress/regress.so', 'nosuchsymbol'; ERROR: could not find function "nosuchsymbol" in file "/home/kaigai/tmp/e/pgsql/src/test/regress/regress.so " --- 72,78 ---- ERROR: only one AS item needed for language "sql" CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C AS 'nosuchfile'; ! ERROR: SELinux: could not get context of nosuchfile CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C AS '/home/kaigai/tmp/e/pgsql/src/test/regress/regress.so', 'nosuchsymbol'; ERROR: could not find function "nosuchsymbol" in file "/home/kaigai/tmp/e/pgsql/src/test/regress/regress.so " ====================================================================== *** ./expected/sanity_check.out Sun Nov 25 12:49:12 2007 --- ./results/sanity_check.out Fri Jun 20 14:55:31 2008 *************** *** 111,116 **** --- 111,117 ---- pg_pltemplate | t pg_proc | t pg_rewrite | t + pg_security | t pg_shdepend | t pg_shdescription | t pg_statistic | t *************** *** 149,155 **** timetz_tbl | f tinterval_tbl | f varchar_tbl | f ! (138 rows) -- -- another sanity check: every system catalog that has OIDs should have --- 150,156 ---- timetz_tbl | f tinterval_tbl | f varchar_tbl | f ! (139 rows) -- -- another sanity check: every system catalog that has OIDs should have -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai(a)ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: KaiGai Kohei on 26 Jun 2008 11:20 Hi, The following patch set (r926) are updated one toward the latest CVS head, and contains some fixes in security policy and documentation. I want to push them for the reviewing queue of CommitFest:Jul. [1/4] Core facilities of PGACE/SE-PostgreSQL http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r926.patch [2/4] "--enable-selinux" option of pg_dump/pg_dumpall http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r926.patch [3/4] Default security policy for SE-PostgreSQL http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r926.patch [4/4] Documentation updates http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r926.patch Thanks, KaiGai Kohei wrote: > Hi, > > The following patch set is our second proposals of SE-PostgreSQL. > > It contains many of fixes and improvements from the previous version. > Please add them a reviwing queue of the next commit fest. > > Thanks, > > List of Patches > =============== > > [1/4] Core facilities of PGACE/SE-PostgreSQL > http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r914.patch > > [2/4] "--enable-selinux" option of pg_dump/pg_dumpall > http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r914.patch > > [3/4] Default security policy for SE-PostgreSQL > http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r914.patch > > [4/4] Documentation updates > http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r914.patch > > We can provide a quick overview for SE-PostgreSQL at: > http://code.google.com/p/sepgsql/wiki/WhatIsSEPostgreSQL > http://sepgsql.googlecode.com/files/PGCON20080523.pdf > > Compile and Installation > ======================== > > The following items are requirements of SE-PostgreSQL. > - Fedora 8 or later system > - SELinux is enabled, and working > - kernel-2.6.23 or later > - selinux-policy and selinux-policy-devel v3.0.8 or later > - libselinux, policycoreutils, checkpolicy > > The followings are step by step installation. > > $ cvs -z3 -d :pserver:anoncvs(a)anoncvs.postgresql.org:/projects/cvsroot \ > export -r HEAD -d pgsql > $ cd pgsql > $ patch -p1 < sepostgresql-sepgsql-8.4devel-3-r914.patch > $ patch -p1 < sepostgresql-pg_dump-8.4devel-3-r914.patch > $ patch -p1 < sepostgresql-policy-8.4devel-3-r914.patch > $ patch -p1 < sepostgresql-docs-8.4devel-3-r914.patch > $ ./configure --enable-selinux > $ make > $ make -C ./contrib/sepgsql_policy > > $ su > # /usr/sbin/semodule -i ./contrib/sepgsql_policy/sepostgresql.pp ... [1] > # make install > # /sbin/restorecon -R /usr/local/pgsql > > $ mkdir -p $PGDATA > $ chcon -t postgresql_db_t -R $PGDATA > $ initdb > $ pg_ctl start > > [1] If "selinux-policy-3.4.2" or later is installed on your system, > install "sepostgresql-devel.pp" instead. > In this version, most of SE-PostgreSQL's policy are got mainlined. > > > Updates from the previous version > ================================= > > o A new type of "security_label" has gone > > In the previous one, "security_context" system column is declared as > security_label type. This type had its input handler, and it translated > a given text representation into an internal Oid value with looking up > pg_security system catalog. If it's not found, the input handler inserts > a new entry automatically. > > The following query can show the reason why this design is problematic. > > SELECT 'system_u:object_r:sepgsql_db_t'::security_label; > > This query seems to us read-only, but it has a possibility to insert > a new tuple into pg_security implicitly. > > In this version, "security_context" system column is re-defined as a TEXT > type attribute, and a given text representation is translated into internal > identifier (Oid) just before insert or update a tuple. This design change > enables to make sure pg_security is not modified in read-only queries. > > > o Query modification has gone. > > In the previous one, SE-PostgreSQL modified WHERE/JOIN ON clause to apply > tuple-level access controls, but its implementation is a bit complicated. > In addition, this design had a possibility to conflict with a new MS patent. > > Now we put a hook on ExecScan to apply tuple-level access controls. > It enables to reduce code complexity and avoid patent conflicts. > > > o Scanning with SnapshotSelf has gone. > > In the previous one, we had to scan some system catalogs with SnapshotSelf > mode at three points (pg_class, pg_largeobject and pg_security). > > * When we defines a relation on heap_create_with_catalog(), a tuple of > pg_class and several tuples of pg_attribute are inserted within same > command id. > A tuple of pg_class has to be refered just before inserting tuples of > pg_attribute, because a new column inherits the security context of its > parent relation in the default. But we cannot find it because these are > inserted within same command id and SnapshotNow scanning mode ignores > these tuples. We cannot invoke CommandIdIncrement() here, because it > finally checks integrity of relation cache, but the relation is not > constructed yet. > > We can apply two option here. One is preserving the security context > of parent table and applying it later without looking up pg_class. > The other is to insert a temporary entry into SysCache until it is > invalidated. > The later approach can also be applied on the next situation, so we > now put InsertSysCache() withing heap_create_with_catalog() to refer > the new tuple before next CommandIdIncrement() which is invoked just > after hecp_create_with_catalog(). > > * When a user gives a security context in text representation, it is > translated into an internal identifier which indicates the oid of > pg_security system catalog. If it was not found, PGACE/SE-PostgreSQL > inserts a new tuple and applies its oid as an internal identifier. > If a same new security context is given within same currentCommandId > twice or more, it tries to insert a new tuple into pg_security twice > or more. However, it violates uniqueness constraint at oid of pg_security. > > Thus, we had to look up pg_security with SnapshotSelf scanning mode > as a fallback when SearchSysCache() returns invalid tuple. But we can > apply same approach here. So, InsertSysCache() is invoked to keep > a newly inserted security context until next CommandIdIncrement(). > > * When a user inserts or deletes a tuple within pg_largeobject directly, > it can also means create a new larageobject, or drop ones. > In SE-PostgreSQL model, it requires 'create' or 'drop' permission, > so we had to check whether the tuple is the first/last one, or not. > > In this case, we assumes inserting a tuple into pg_largeobject directly > has a possibility to create new largeobject, and 'create' permission > should be always evaluated, not only 'write'. > This assumption kills to scan pg_largeobject for each insertion/deletion. > > If requests come from lowrite() or lo_create(), we can distinguish its > meanings, so proper permissions are applied in the most cases. > > I guess the InsertSysCache() will be an arguable interface, but it can resolve > our problem with minimum impact and utilizing existing facilities, so it is > better than any other solutuions. > > > o A new guc parameter to enable/disable SE-PostgreSQL > > It can take four options, as follows: > sepostgresql = [ default | enforcing | permissive | disabled ] > - default: always follows kernel setting (default) > - enforcing: works in enforcing mode (MAC and audit enabled). > - permissive: works in permissive mode (audit log only). > - disabled: disables SE-PostgreSQL feature. > > > o PGACE hooks are inlined > > The contains of src/backend/security/pgaceHooks.c are moved to > src/include/security/pgace.h and inlined. > It enables to reduce cost to invoke empty function when this > feature is disabled. > > > o Generic writable system column > > SystemAttributeIsWritable() can abstract what system attribute is writable. > (Currently, the security system catalog is the only one writable.) > If it returns true on the target of INSERT, UPDATE or SELECT INTO, these > TargetEntries are marked as junk, and we can fetch its value on ExecutorRun() > separated from any other regular attribute. > > > o early security design > > In the previous one, we stores a relationship between security id and > text representation on bootstraping mode, because pg_security system > catalog is not constructed yet in this time. > The current version holds them in-memory cache and writes out on the tail > of the bootstraping process. > > > o Documentation updates > > The doc/src/sgml/security.sgml gives us a short description of SE-PostgreSQL > and PGACE security framework. In addition, we added source code comments for > most of functions, including PGACE hooks. > > > o Miscellaneous updates > * Two separated patches (pgace and sepgsql) are integrated into one. > * Copyrights are changed to "PostgreSQL Global Development Group". > * Several PGACE hooks are added, redefined and removed. > * Now, we can run regression test without any problem, except for two > tests which can be explained reasonably. > * SELinux state monitoring process is implemented using an existing > facilities provided by postmaster.c. > * Coding styles are fixed. > * A definition of LWlock is moved to storage/lwlock.h > * Definitions of SEvalItemXXXX are moved to nodes/nodes.h > * Compiler warnings come from SE-PostgreSQL are killed. > * Some error codes are added within 'SE' class, and elog()s which can > report user facing messages are replaced by ereport(). > * Shell function is removed from genbki.sh. > * Default security context of files consider --prefix setting in > configure script. > > > Regression Tests > ================ > > Now we remain two test fails, but these can be explained reasonably. > > The first fail (create_function_1) means that SE-PostgreSQL detects > a client attempt to load an invalid file before core PostgreSQL doing, > and generates its error message. > > The later one (sanity_check) means the regression test can detect > an increation of system catalogs correctly. > > (*) Turn on "sepgsql_regression_test_mode" boolean of SELinux before > regression test. It enables you to load shared library modules > installed under user's home directory. > > # /usr/sbin/setsebool sepgsql_regression_test_mode on > > $ make check > : > (snip) > : > ======================== > 2 of 115 tests failed. > ======================== > > > [kaigai(a)saba pgsql]$ less src/test/regress/regression.diffs > *** ./expected/create_function_1.out Fri Jun 20 14:55:12 2008 > --- ./results/create_function_1.out Fri Jun 20 14:55:28 2008 > *************** > *** 72,78 **** > ERROR: only one AS item needed for language "sql" > CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C > AS 'nosuchfile'; > ! ERROR: could not access file "nosuchfile": No such file or directory > CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C > AS '/home/kaigai/tmp/e/pgsql/src/test/regress/regress.so', 'nosuchsymbol'; > ERROR: could not find function "nosuchsymbol" in file "/home/kaigai/tmp/e/pgsql/src/test/regress/regress.so > " > --- 72,78 ---- > ERROR: only one AS item needed for language "sql" > CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C > AS 'nosuchfile'; > ! ERROR: SELinux: could not get context of nosuchfile > CREATE FUNCTION test1 (int) RETURNS int LANGUAGE C > AS '/home/kaigai/tmp/e/pgsql/src/test/regress/regress.so', 'nosuchsymbol'; > ERROR: could not find function "nosuchsymbol" in file "/home/kaigai/tmp/e/pgsql/src/test/regress/regress.so > " > > ====================================================================== > > *** ./expected/sanity_check.out Sun Nov 25 12:49:12 2007 > --- ./results/sanity_check.out Fri Jun 20 14:55:31 2008 > *************** > *** 111,116 **** > --- 111,117 ---- > pg_pltemplate | t > pg_proc | t > pg_rewrite | t > + pg_security | t > pg_shdepend | t > pg_shdescription | t > pg_statistic | t > *************** > *** 149,155 **** > timetz_tbl | f > tinterval_tbl | f > varchar_tbl | f > ! (138 rows) > > -- > -- another sanity check: every system catalog that has OIDs should have > --- 150,156 ---- > timetz_tbl | f > tinterval_tbl | f > varchar_tbl | f > ! (139 rows) > > -- > -- another sanity check: every system catalog that has OIDs should have > -- KaiGai Kohei <kaigai(a)kaigai.gr.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Peter Eisentraut on 7 Jul 2008 11:39 Am Donnerstag, 26. Juni 2008 schrieb KaiGai Kohei: > The following patch set (r926) are updated one toward the latest CVS head, > and contains some fixes in security policy and documentation. OK, I have quickly read through these patches. They look very nice, so I am optimistic we can get through this. First of all, now would be a good time if someone out there really wants to object to this feature in general. It will probably always be a niche feature. But all the code is hidden behind ifdefs or other constructs that a compiler can easily hide away (or we can make it so, at least). Here is a presentation from PGCon on SE-PostgreSQL: http://www.pgcon.org/2008/schedule/events/77.en.html Are there any comments yet from the (Trusted)Solaris people that wanted to evaluate this approach for compatibility with their approach? In general, are we OK with the syntax CONTEXT = '...'? I would rather see something like SECURITY CONTEXT '...'. There are a lot of contexts, after all. This will also add a system column called security_context. I think that is OK. In the pg_dump patch: spelling mistake "tuen on/off" Evil coding style: if (strcmp(SELINUX_SYSATTR_NAME, security_sysattr_name)) -- compare the result with 0 please. The above code appears to assume that security_sysattr_name never changes, but then why do we need a GUC parameter to show it? Might want to change the option name --enable-selinux to something like --security-context. In general, we might want to not name things selinux_* but instead sepostgresql_* or security_* or security_context_*. Or maybe PGACE? On the default policy: Should this really be a contrib module? Considering that it would be a core feature that is not really usable without a policy. Please change all the sepgsql_* things to sepostgresql_*, considering that you are using both already, so we shouldn't have both forms of names. Documentation: Looks good for a start, but we will probably want to write more later. -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Josh Berkus on 7 Jul 2008 13:28 Peter, > Are there any comments yet from the (Trusted)Solaris people that wanted to > evaluate this approach for compatibility with their approach? In case they're not paying attention, a month ago the Trusted Solaris folks said the general approach was fine, but that they would likely want to extend SEPostgres further (that is, add to it) for 8.5. --Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: KaiGai Kohei on 8 Jul 2008 00:26
Thanks for your reviewing. Peter Eisentraut wrote: > Am Donnerstag, 26. Juni 2008 schrieb KaiGai Kohei: >> The following patch set (r926) are updated one toward the latest CVS head, >> and contains some fixes in security policy and documentation. > > OK, I have quickly read through these patches. They look very nice, so I am > optimistic we can get through this. > > First of all, now would be a good time if someone out there really wants to > object to this feature in general. It will probably always be a niche > feature. But all the code is hidden behind ifdefs or other constructs that a > compiler can easily hide away (or we can make it so, at least). > > Here is a presentation from PGCon on SE-PostgreSQL: > http://www.pgcon.org/2008/schedule/events/77.en.html > > Are there any comments yet from the (Trusted)Solaris people that wanted to > evaluate this approach for compatibility with their approach? In this April, I had a face-to-face meeting with Trusted Solaris people to discuss SE-PostgreSQL and PGACE framework, and concluded that PGACE framework provides enough facilities for both operating systems. I modified several hooks from CommitFest:May, however, its fundamental structures are unchanged. > In general, are we OK with the syntax CONTEXT = '...'? I would rather see > something like SECURITY CONTEXT '...'. There are a lot of contexts, after > all. If we change it, I prefer SECURITY_CONTEXT = '...' style, because it enables to represent the left hand with a single token and make PGACE hooks simpler. I also agree the CONTEXT has widespread meanings and to be changed here. > This will also add a system column called security_context. I think that is > OK. Thanks, > In the pg_dump patch: > > spelling mistake "tuen on/off" I'll fix it soon. > Evil coding style: if (strcmp(SELINUX_SYSATTR_NAME, security_sysattr_name)) -- > compare the result with 0 please. OK, I'll fix it and check my implementations in other place. > The above code appears to assume that security_sysattr_name never changes, but > then why do we need a GUC parameter to show it? The purpose of the GUC parameter is to identify the kind of security feature if enabled. It can be changed by other security features, and it will show us different value. > Might want to change the option name --enable-selinux to something > like --security-context. > > In general, we might want to not name things selinux_* but instead > sepostgresql_* or security_* or security_context_*. Or maybe PGACE? The pgace_* scheme is an attractive idea, although the server process has to provide a bit more hints (like the name of security system column and the kind of objects exported with security attribute) pg_dump to support various kind of security features with smallest implementation. If not so, I prefer the combination of --security-context and sepostgresql_*. > On the default policy: > > Should this really be a contrib module? Considering that it would be a core > feature that is not really usable without a policy. It is correct, SE-PostgreSQL feature always need its security policy. Do you think "src/backend/security/sepgsql/policy" is better? > Please change all the sepgsql_* things to sepostgresql_*, considering that you > are using both already, so we shouldn't have both forms of names. We can convert them from sepostgresql_* to sepgsql_* easily, because the longer forms are not used as a part of identifier in security context. But we have a possible matter in changing from sepgsql_* to sepostgresql_*. Here is a news from SELinux community: http://marc.info/?l=selinux&m=121501336024075&w=2 It shows most part of the SE-PostgreSQL policy module got merged into the upstreamed at selinux-policy-3.4.2 or later. It contains identifier with sepgsql_* stuff, so it has a possible matter when users upgrade his security policy. If their database is labeled as sepostgresql_*, it will lose rules defined in the policy when users upgrade selinux-policy package to the latest one. > Documentation: > > Looks good for a start, but we will probably want to write more later. Do you think what kind of information should be added? I intentionally omitted descriptions about SELinux itself, because it is a documentation of PostgreSQL, not OS. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei <kaigai(a)ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |