Re: sepgsql contrib module
sepgsql contrib module
От:
KaiGai Kohei <kaigai@ak.jp.nec.com>
Дата:
The attached patch is the modular version of SE-PostgreSQL. Since I reduced the caching mechanism for access control decision, its code scale became about 2.6KL. [kaigai@saba sepgsql]$ wc -l *.[ch] 353 dml.c 366 hooks.c 477 label.c 158 proc.c 267 relation.c 98 schema.c 617 selinux.c 287 sepgsql.h 2623 total In addition, *.sgml file uses about 300 lines. There is one another issue to be discussed. We need a special form of regression test. Because SE-PostgreSQL makes access control decision based on security label of the peer process, we need to switch psql process during regression test. (So, I don't include test cases yet.) We have 'runcon' command to launch a child process with specified security label as long as the security policy allows. If we could launch 'psql' by 'runcon' with specified label, we can describe test-cases on the existing framework on 'make installcheck'. An idea is to add an option to pg_regress to launch psql command with a specified wrapper program (like 'runcon'). In this case, each contrib modules kicks with REGRESS_OPTS setting. One thing to be considered is the security label to be given to the 'runcon' is flexible for each *.sql files. Thanks, -- KaiGai Kohei
Re: sepgsql contrib module
От:
KaiGai Kohei <kaigai@ak.jp.nec.com>
Дата:
(2010/12/24 11:53), KaiGai Kohei wrote: > There is one another issue to be discussed. > We need a special form of regression test. Because SE-PostgreSQL > makes access control decision based on security label of the peer > process, we need to switch psql process during regression test. > (So, I don't include test cases yet.) > > We have 'runcon' command to launch a child process with specified > security label as long as the security policy allows. If we could > launch 'psql' by 'runcon' with specified label, we can describe > test-cases on the existing framework on 'make installcheck'. > > An idea is to add an option to pg_regress to launch psql command > with a specified wrapper program (like 'runcon'). > In this case, each contrib modules kicks with REGRESS_OPTS setting. > One thing to be considered is the security label to be given to > the 'runcon' is flexible for each *.sql files. > The attached patch adds --launcher=COMMAND option to pg_regress. If a command was specified, pg_regress prepends the specified command string in front of psql command. When we use this option, psql command process will launched via the launcher program. Of course, the launcher has responsibility to execute psql correctly.) This example is a case when I run a regression test on cube module. It tries to launch psql using 'runcon -l s0'. [kaigai@saba cube]$ make installcheck REGRESS_OPTS="--launcher='runcon -l s0' --dbname=cube_regress" make -C ../../src/test/regress pg_regress make[1]: Entering directory `/home/kaigai/repo/pgsql/src/test/regress' make -C ../../../src/port all make[2]: Entering directory `/home/kaigai/repo/pgsql/src/port' make[2]: Nothing to be done for `all'. make[2]: Leaving directory `/home/kaigai/repo/pgsql/src/port' make[1]: Leaving directory `/home/kaigai/repo/pgsql/src/test/regress' ../../src/test/regress/pg_regress --inputdir=. --psqldir=/usr/local/pgsql/bin --launcher='runcon -l s0' --dbname=cube_regress cube (using postmaster on Unix socket, default port) ============== dropping database "cube_regress" ============== DROP DATABASE ============== creating database "cube_regress" ============== CREATE DATABASE ALTER DATABASE ============== running regression test queries ============== test cube ... ok ===================== All 1 tests passed. ===================== During the regression test, I checked security context of the process. [kaigai@saba ~]$ env LANG=C pstree -Z systemd(`system_u:system_r:init_t:s0') : |-sshd(`unconfined_u:system_r:sshd_t:s0-s0:c0.c1023') | |-sshd(`unconfined_u:system_r:sshd_t:s0-s0:c0.c1023') | | `-sshd(`unconfined_u:system_r:sshd_t:s0-s0:c0.c1023') | | `-bash(`unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023') | | `-make(`unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023') | | `-pg_regress(`unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023') | | `-psql(`unconfined_u:unconfined_r:unconfined_t:s0') It shows us the launcher program drops privileges of "c0.c1023" on end of the security label of processes between pg_regress and psql. How about the idea to implement regression test for SE-PostgreSQL, or possible other stuff which depends on environment variables. Thanks, -- KaiGai Kohei
Re: sepgsql contrib module
От:
KaiGai Kohei <kaigai@ak.jp.nec.com>
Дата:
The attached patch is the modular version of SE-PostgreSQL (take.2). Its patch scale grew up to 4KL because of regression test inclusion, although code size was not changed (2.6KL). I had to add a small piece into pg_regress to launch psql command using a launcher program that kicks psql with controlled privilege set, because SE-PostgreSQL makes access control decision based on security label of the peer process. This enhancement allows to implement regression test according to the framework currently we have, so additional setups to run regression test got simplified. I found several bugs during code revising, these were also killed. How about feasibility to merge this 4KL chunks during the rest of 45 days? I think we should decide this general direction at first. Simon, A section of "Guide to Security Labels" is now under describing. Please wait for a few days to revise documentation a bit more. Thanks, $ cat ~/sepgsql-v9.1-lite.2.patch | diffstat configure | 122 +++++++ configure.in | 13 contrib/Makefile | 4 contrib/README | 4 contrib/sepgsql/Makefile | 25 + contrib/sepgsql/dml.c | 353 +++++++++++++++++++++ contrib/sepgsql/expected/dml.out | 178 ++++++++++ contrib/sepgsql/expected/label.out | 109 ++++++ contrib/sepgsql/hooks.c | 366 +++++++++++++++++++++ contrib/sepgsql/label.c | 477 ++++++++++++++++++++++++++++ contrib/sepgsql/launcher | 52 +++ contrib/sepgsql/proc.c | 158 +++++++++ contrib/sepgsql/relation.c | 267 +++++++++++++++ contrib/sepgsql/schema.c | 98 +++++ contrib/sepgsql/selinux.c | 618 +++++++++++++++++++++++++++++++++++++ contrib/sepgsql/sepgsql-regtest.te | 59 +++ contrib/sepgsql/sepgsql.h | 287 +++++++++++++++++ contrib/sepgsql/sepgsql.sql.in | 36 ++ contrib/sepgsql/sql/dml.sql | 114 ++++++ contrib/sepgsql/sql/label.sql | 73 ++++ doc/src/sgml/contrib.sgml | 1 doc/src/sgml/filelist.sgml | 1 doc/src/sgml/sepgsql.sgml | 468 ++++++++++++++++++++++++++++ src/Makefile.global.in | 1 src/test/regress/pg_regress.c | 6 src/test/regress/pg_regress.h | 1 src/test/regress/pg_regress_main.c | 7 27 files changed, 3897 insertions(+), 1 deletion(-) (2010/12/24 11:53), KaiGai Kohei wrote: > The attached patch is the modular version of SE-PostgreSQL. > > Since I reduced the caching mechanism for access control decision, > its code scale became about 2.6KL. > > [kaigai@saba sepgsql]$ wc -l *.[ch] > 353 dml.c > 366 hooks.c > 477 label.c > 158 proc.c > 267 relation.c > 98 schema.c > 617 selinux.c > 287 sepgsql.h > 2623 total > > In addition, *.sgml file uses about 300 lines. > > > There is one another issue to be discussed. > We need a special form of regression test. Because SE-PostgreSQL > makes access control decision based on security label of the peer > process, we need to switch psql process during regression test. > (So, I don't include test cases yet.) > > We have 'runcon' command to launch a child process with specified > security label as long as the security policy allows. If we could > launch 'psql' by 'runcon' with specified label, we can describe > test-cases on the existing framework on 'make installcheck'. > > An idea is to add an option to pg_regress to launch psql command > with a specified wrapper program (like 'runcon'). > In this case, each contrib modules kicks with REGRESS_OPTS setting. > One thing to be considered is the security label to be given to > the 'runcon' is flexible for each *.sql files. > > Thanks, > -- KaiGai Kohei
Re: sepgsql contrib module
От:
KaiGai Kohei <kaigai@ak.jp.nec.com>
Дата:
The attached patch is a revised version.
Changeset from the previous revision:
- It fixed up a typo in catalog.
The "process:{transition}" is correct permission name.
- Add checks to avoid inlining function without db_procedure:{execute}
permission. Sorry, process:{transition} shall be checked in other place.
- sepgsql_utility_command() was added as a guest of ProcessUtility_hook,
to control LOAD command, right now.
- Documentation was revised. Mostly, description about permission checks.
- Some mixture of tabs/spaces were fixed.
- Source code comments were revised getting more friendly to pgindent,
as follows:
+/*
+ * sepgsql_mode
+ *
+ * SEPGSQL_MODE_DISABLED: Disabled on runtime
+ * SEPGSQL_MODE_DEFAULT: Same as system settings
+ * SEPGSQL_MODE_PERMISSIVE: Always permissive mode
+ * SEPGSQL_MODE_INTERNAL: Same as permissive, except for no audit logs
+ */
I also tried to run pgindent on the source files. Some of them were revised
well according to the coding rule, but some of them were painful, like:
{
- "db_schema", SEPG_CLASS_DB_SCHEMA,
+ "db_schema", SEPG_CLASS_DB_SCHEMA,
{
- { "create", SEPG_DB_SCHEMA__CREATE },
- { "drop", SEPG_DB_SCHEMA__DROP },
- { "getattr", SEPG_DB_SCHEMA__GETATTR },
- { "setattr", SEPG_DB_SCHEMA__SETATTR },
- { "relabelfrom", SEPG_DB_SCHEMA__RELABELFROM },
- { "relabelto", SEPG_DB_SCHEMA__RELABELTO },
- { "search", SEPG_DB_SCHEMA__SEARCH },
- { "add_name", SEPG_DB_SCHEMA__ADD_NAME },
- { "remove_name", SEPG_DB_SCHEMA__REMOVE_NAME },
- { NULL, 0UL },
- }
+ {
+ "create", SEPG_DB_SCHEMA__CREATE},
+ {
+ "drop", SEPG_DB_SCHEMA__DROP},
+ {
+ "getattr", SEPG_DB_SCHEMA__GETATTR},
+ {
+ "setattr", SEPG_DB_SCHEMA__SETATTR},
+ {
+ "relabelfrom", SEPG_DB_SCHEMA__RELABELFROM},
+ {
+ "relabelto", SEPG_DB_SCHEMA__RELABELTO},
+ {
+ "search", SEPG_DB_SCHEMA__SEARCH},
+ {
+ "add_name", SEPG_DB_SCHEMA__ADD_NAME},
+ {
+ "remove_name", SEPG_DB_SCHEMA__REMOVE_NAME},
+ {
+ NULL, 0UL},}
},
Do we have any workaround to avoid these indenting/formatting?
Or, the reformatted code is better than before?
Thanks,
(2011/01/07 12:02), Robert Haas wrote:
> 2011/1/6 KaiGai Kohei:
>> If we use result of the `pg_config --sharedir` here, how about this
>> writing style? Or, do we have any other ideas?
>
> I'm not sure - I'll look at your next draft more closely.
>
>> The background of this wikipage is that I was persuading people
>> this feature being worthful, so the contents tend to philosophical
>> things rather than actual specifications.
>
> Yeah.
>
>> I also think wiki page allows us to brush up the documentation
>> rather than exchanging patches effectively. I'll set up a wiki page
>> that contains same contents with *.sgml file to revise documentation
>> stuff to be included into the *.sgml file finally. How about this idea?
>
> Sounds good.
>
--
KaiGai Kohei
Re: sepgsql contrib module
От:
KaiGai Kohei <kaigai@ak.jp.nec.com>
Дата:
(2011/01/27 0:25), Robert Haas wrote: > 2011/1/25 KaiGai Kohei: >> (2011/01/26 12:23), KaiGai Kohei wrote: >>>>> Yikes. On further examination, exec_object_restorecon() is pretty >>>>> bogus. Surely you need some calls to quote_literal_cstr() in there >>>>> someplace. >>>> >>> Are you concerning about the object name being supplied to >>> selabel_lookup_raw() in exec_object_restorecon()? >>> I also think this quoting you suggested is reasonable. >>> >> How about the case when the object name only contains alphabet and >> numerical characters? > > Oh, quote_literal_cstr() is the wrong function - these are > identifiers, not literals. So we should use quote_identifier(). > OK, I did with quote_identifier(). The attached patch fixes up several stuffs in sepgsql module. - The object names being supplied to selabel_lookup_raw() to lookup initial labels become qualified by quote_identifier(), if necessary. - On access violation, sepgsql_check_perms() records audit logs. It contains object name being referenced. It became generated using getObjectDescription(). - Also, sepgsql_audit_log() becomes to quote the supplied object name, because it may contains white-space. - Error messages become obtaining "%m", when the error was originated from the libselinux interfaces. It will provides DBA a hint why interactions with SELinux does not work well. - Documentation was updated to suggest users to install libselinux v2.0.93 or later, because it used newer features than ones provided in v2.0.80. - Regression Test was updated, because of error message updates. Thanks, -- KaiGai Kohei
Re: sepgsql contrib module
От:
KaiGai Kohei <kaigai@ak.jp.nec.com>
Дата:
(2011/01/27 22:26), Robert Haas wrote: > 2011/1/27 KaiGai Kohei: >> - Error messages become obtaining "%m", when the error was >> originated from the libselinux interfaces. It will provides >> DBA a hint why interactions with SELinux does not work well. > > No space before the ": %m", please. > > Also this word looks like a typo: seuciryt > The attached patch eliminated spaces before ": %m", and fixed up the typo. Thanks, -- KaiGai Kohei
Re: sepgsql contrib module
От:
Kohei Kaigai <Kohei.Kaigai@EU.NEC.COM>
Дата:
The attached patch removes rules to build a policy package for regression test and modifies documentation part to introduce steps to run the test. Thanks, -- NEC Europe Ltd, Global Competence Center KaiGai Kohei > -----Original Message----- > From: Kohei Kaigai > Sent: 15 February 2011 18:27 > To: 'Robert Haas'; Tom Lane > Cc: Andrew Dunstan; Stephen Frost; KaiGai Kohei; PgHacker > Subject: RE: [HACKERS] sepgsql contrib module > > > > > -----Original Message----- > > From: Robert Haas [mailto:robertmhaas@gmail.com] > > Sent: 15 February 2011 16:52 > > To: Tom Lane > > Cc: Andrew Dunstan; Kohei Kaigai; Stephen Frost; KaiGai Kohei; PgHacker > > Subject: Re: [HACKERS] sepgsql contrib module > > > > On Tue, Feb 15, 2011 at 11:41 AM, Tom Lane wrote: > > > Robert Haas writes: > > >> On Tue, Feb 15, 2011 at 11:01 AM, Tom Lane wrote: > > >>> Robert Haas writes: > > >>>> Those are good points. My point was just that you can't actually > > >>>> build that file at the time you RUN the regression tests, because > you > > >>>> have to build it first, then install it, then run the regression > > >>>> tests. It could be a separate target, like 'make policy', but I > don't > > >>>> think it works to make it part of 'make installcheck'. > > > > > >>> So? Once you admit that you can do that, it's a matter of a couple > > more > > >>> lines to make the installcheck target depend on the policy target > iff > > >>> selinux was enabled. > > > > > >> Sure, you could do that, but I don't see what problem it would fix. > > >> You'd still have to build and manually install the policy before you > > >> could run make installcheck. And once you've done that, you don't > > >> need to rebuild it every future time you run make installcheck. > > > > > > Oh, I see: you're pointing out the root-only "semodule" step that has > > to > > > be done in between there. Good point. But the current arrangement > is > > > still a mistake: the required contents of sepgsql-regtest.pp depend > on > > > the configuration of the test system, which can't be known at build > > > time. > > > > > > So what we should do is offer a "make policy" target and alter the test > > > instructions to say you should do that and then run semodule. Or maybe > > > just put the whole "make -f /usr/share/selinux/devel/Makefile" dance > > > into the instructions --- it doesn't look to me like our makefile > > > infrastructure really has anything useful to add to that. > > > > Yeah, agreed. > > > I also agree with this direction. The policy type depends on individual > installations, > it is not easy to assume on build time. > Please wait for a small patch to remove this rule from Makefile and update > documentation. > > As a side note, we can have a build option that does not require selinux > enabled. > The reason why Makefile of selinux tries to /selinux/mls is that we don't > specify > MLS=1 or MLS=0 explicitly. > IIRC, the specfile of RHEL/Fedora gives all the Makefile parameters > explicitly, thus, > selinux does not need to be enabled on the build server. > However, it is not a solution in this case. It is not easy to estimate the > required > policy type and existence of MLS support on build time. > > Thanks, > -- > NEC Europe Ltd, Global Competence Center > KaiGai Kohei
