sepgsql contrib module

(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 <kaigai@ak.jp.nec.com>

On Fri, 2010-12-24 at 11:53 +0900, KaiGai Kohei wrote:

The attached patch is the modular version of SE-PostgreSQL.

Looks interesting.

Couple of thoughts...

Docs don't mention row-level security. If we don't have it, I think we
should say that clearly.

I think we need a "Guide to Security Labels" section in the docs. Very
soon, because its hard to know what is being delivered and what is not.

Is the pg_seclabel table secure? Looks like the labels will be available
to read.

How do we tell if sepgsql is installed?

What happens if someone alters the configuration so that the sepgsql
plugin is no longer installed. Does the hidden data become visible?

Thanks

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

(2010/12/27 17:53), Simon Riggs wrote:

On Fri, 2010-12-24 at 11:53 +0900, KaiGai Kohei wrote:

The attached patch is the modular version of SE-PostgreSQL.

Looks interesting.

Couple of thoughts...

Docs don't mention row-level security. If we don't have it, I think we
should say that clearly.

Indeed, it is a good idea the document mentions what features are not
implemented in this version clearly, not only row-level security, but
DDL permissions and so on. I'd like to revise it soon.

I think we need a "Guide to Security Labels" section in the docs. Very
soon, because its hard to know what is being delivered and what is not.

Does it describe what is security label and the purpose of them?
OK, I'd like to add this section here.

Is the pg_seclabel table secure? Looks like the labels will be available
to read.

If we want to control visibility of each labels, we need row-level
granularity here.

How do we tell if sepgsql is installed?

Check existence of GUC variables of sepgsql.*.

What happens if someone alters the configuration so that the sepgsql
plugin is no longer installed. Does the hidden data become visible?

Yes. If sepgsql plugin is uninstalled, the hidden data become visible.
But no matter. Since only a person who is allowed to edit postgresql.conf
can uninstall it, we cannot uninstall it in run-time.
(An exception is loading a malicious module, but we will be able to
hook this operation in the future version.)

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Thu, 2010-12-30 at 09:26 +0900, KaiGai Kohei wrote:

What happens if someone alters the configuration so that the sepgsql
plugin is no longer installed. Does the hidden data become visible?

Yes. If sepgsql plugin is uninstalled, the hidden data become visible.
But no matter. Since only a person who is allowed to edit postgresql.conf
can uninstall it, we cannot uninstall it in run-time.
(An exception is loading a malicious module, but we will be able to
hook this operation in the future version.)

IMHO all security labels should be invisible if the provider is not
installed correctly.

That at least prevents us from accidentally de-installing a module and
having top secret data be widely available.

If you have multiple providers configured, you need to be careful not to
allow a provider that incorrectly implements the plugin API, so that
prior plugins are no longer effective.

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

(2010/12/30 9:34), Simon Riggs wrote:

On Thu, 2010-12-30 at 09:26 +0900, KaiGai Kohei wrote:

What happens if someone alters the configuration so that the sepgsql
plugin is no longer installed. Does the hidden data become visible?

Yes. If sepgsql plugin is uninstalled, the hidden data become visible.
But no matter. Since only a person who is allowed to edit postgresql.conf
can uninstall it, we cannot uninstall it in run-time.
(An exception is loading a malicious module, but we will be able to
hook this operation in the future version.)

IMHO all security labels should be invisible if the provider is not
installed correctly.

Probably, it needs row-level granularity to control visibility of
each entries of pg_seclabel, because all the provider shares same
system catalog.
So, I don't think this mechanism is feasible right now.

That at least prevents us from accidentally de-installing a module and
having top secret data be widely available.

If you have multiple providers configured, you need to be careful not to
allow a provider that incorrectly implements the plugin API, so that
prior plugins are no longer effective.

Yep. It is responsibility of DBA who tries to set up security providers.
DBA has to install only trustable or well-debugged modules (not limited
to security providers) to avoid troubles.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

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 <kaigai@ak.jp.nec.com>

2011/1/5 KaiGai Kohei <kaigai@ak.jp.nec.com>:

The attached patch is the modular version of SE-PostgreSQL (take.2).

I'm reading through the documentation and so far it looks pretty
reasonable. But I have some questions and suggested changes, of
course. :-)

1. Why is sepgsql the right name for this module, as opposed to, say,
selinux? We don't call the cube module cubepgsql, or the hstore
module hstorepgsql. Maybe there's a reason why this case is
different, but I'm not sure.

2. The docs contains some references to /usr/local/pgsql/share.. Does
this really mean "whatever sharedir you established when you ran
configure", i.e. the output of pg_config --sharedir? I hope so.

3. The language for the sepgsql.permissive GUC suggests that it's
PGC_POSTMASTER, but I'd think PGC_SIGHUP ought to be sufficient.
Either way, please copy the appropriate language from some existing
GUC of the same type instead of inventing a new way to say it. I also
have no idea what "because it invalidates all the inefficient stuff"
means.

4. Please remove the upcoming features section of the documentation.
This material is appropriate for a page on the wiki, but shouldn't be
part of the official documentation. Instead, you might want to have a
*short* "Limitations" section.

5. I'm not too sure about this one, but I think it might be good to
elaborate on what we mean by respecting the system SE-Linux policy.
What kinds of objects do we support checks on? What sorts of checks?
What kind of access can we allow/deny?

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

(2011/01/06 14:28), Robert Haas wrote:

2011/1/5 KaiGai Kohei<kaigai@ak.jp.nec.com>:

The attached patch is the modular version of SE-PostgreSQL (take.2).

I'm reading through the documentation and so far it looks pretty
reasonable. But I have some questions and suggested changes, of
course. :-)

Thanks for your reviewing in spite of large chunk.

1. Why is sepgsql the right name for this module, as opposed to, say,
selinux? We don't call the cube module cubepgsql, or the hstore
module hstorepgsql. Maybe there's a reason why this case is
different, but I'm not sure.

In some previous cases when SELinux model was ported to other systems,
its project was named as SE-(other system), such as SE-BSD, SE-X, etc...
I named it according to this convention, however, it is indeed uncertain
whether 'sepgsql' follows on the convention in pgsql side.

I don't think it is a strong reason why the module is named as 'sepgsql'
instead of 'selinux'. In advertisement context, we can just call it as
SE-PostgreSQL.

2. The docs contains some references to /usr/local/pgsql/share.. Does
this really mean "whatever sharedir you established when you ran
configure", i.e. the output of pg_config --sharedir? I hope so.

Yes, it means the sharedir being configured.

I found the following description at the installation.sgml.
I should put this kind of mention on the documentation.

| <para>
| These instructions assume that your existing installation is under the
| <filename>/usr/local/pgsql</> directory, and that the data area is in
| <filename>/usr/local/pgsql/data</>. Substitute your paths
| appropriately.
| </para>

3. The language for the sepgsql.permissive GUC suggests that it's
PGC_POSTMASTER, but I'd think PGC_SIGHUP ought to be sufficient.
Either way, please copy the appropriate language from some existing
GUC of the same type instead of inventing a new way to say it. I also
have no idea what "because it invalidates all the inefficient stuff"
means.

OK, I'll try to find up similar description then fix up both of the
code and documentation.

4. Please remove the upcoming features section of the documentation.
This material is appropriate for a page on the wiki, but shouldn't be
part of the official documentation. Instead, you might want to have a
*short* "Limitations" section.

OK, I'll replace an itemization of limitations in this version.

5. I'm not too sure about this one, but I think it might be good to
elaborate on what we mean by respecting the system SE-Linux policy.
What kinds of objects do we support checks on? What sorts of checks?
What kind of access can we allow/deny?

I guess these detailed description makes amount of this chapter
disproportionally increase in the future version.
My preference is wikipage to provide this kind of detailed information.

http://wiki.postgresql.org/wiki/SEPostgreSQL

The contents of above wikipage is now obsoleted, because it assumed
SELinux support as a built-in feature. But it is a good time to fix
up the description.

Thanks,
--
KaiGai Kohei <kaigai@ak.jp.nec.com>

2011/1/6 KaiGai Kohei <kaigai@ak.jp.nec.com>:

1. Why is sepgsql the right name for this module, as opposed to, say,
selinux?  We don't call the cube module cubepgsql, or the hstore
module hstorepgsql.  Maybe there's a reason why this case is
different, but I'm not sure.

In some previous cases when SELinux model was ported to other systems,
its project was named as SE-(other system), such as SE-BSD, SE-X, etc...
I named it according to this convention, however, it is indeed uncertain
whether 'sepgsql' follows on the convention in pgsql side.

OK. If there's an existing convention of calling things
SE-<productname> then let's do the same thing here. As long as it's
documented clearly it shouldn't be a problem.

2. The docs contains some references to /usr/local/pgsql/share..  Does
this really mean "whatever sharedir you established when you ran
configure", i.e. the output of pg_config --sharedir?  I hope so.

Yes, it means the sharedir being configured.

I found the following description at the installation.sgml.
I should put this kind of mention on the documentation.

|  <para>
|   These instructions assume that your existing installation is under the
|   <filename>/usr/local/pgsql</> directory, and that the data area is in
|   <filename>/usr/local/pgsql/data</>.  Substitute your paths
|   appropriately.
|  </para>

Why does the user need to know about this at all? Doesn't make
install put everything in the right place?

5. I'm not too sure about this one, but I think it might be good to
elaborate on what we mean by respecting the system SE-Linux policy.
What kinds of objects do we support checks on?  What sorts of checks?
What kind of access can we allow/deny?

I guess these detailed description makes amount of this chapter
disproportionally increase in the future version.
My preference is wikipage to provide this kind of detailed information.

 http://wiki.postgresql.org/wiki/SEPostgreSQL

The contents of above wikipage is now obsoleted, because it assumed
SELinux support as a built-in feature. But it is a good time to fix
up the description.

I'd prefer to have it in the actual documentation. I think
SE-PostgreSQL is going to need a lot of documentation. A wiki page
risks getting out of date or having the wrong information for the
version the user has installed. 9.1 may be quite different from 9.2,
for example.

I wouldn't worry about the scale of the patch too much as far as
documentation goes. In reviewing previous patches you've written,
I've often cut large amounts of documentation that I didn't think were
important enough to be worth including (and most of the contents of
the upcoming features section falls into that category). But that
doesn't really take that much time, and it's certainly a lot easier to
remove some extra documentation than it is to write something that's
missing. Most of what you have here right now describes why you might
want to use this feature, rather than what the feature actually does.
If you want to start by updating the wiki page, that's fine, and may
be an easier way for us to collaborate than doing it by exchanging
patches. But ultimately I think it needs to go in the docs.

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

2. The docs contains some references to /usr/local/pgsql/share.. Does
this really mean "whatever sharedir you established when you ran
configure", i.e. the output of pg_config --sharedir? I hope so.

Yes, it means the sharedir being configured.

I found the following description at the installation.sgml.
I should put this kind of mention on the documentation.

|<para>
| These instructions assume that your existing installation is under the
|<filename>/usr/local/pgsql</> directory, and that the data area is in
|<filename>/usr/local/pgsql/data</>. Substitute your paths
| appropriately.
|</para>

Why does the user need to know about this at all? Doesn't make
install put everything in the right place?

It seemed to me a convenient writing-style to inform users an installation
path of file. What I like to write is showing users what path stores what
files needed in installation process.

If we use result of the `pg_config --sharedir` here, how about this
writing style? Or, do we have any other ideas?

<screen>
$ su
# SHAREDIR=`pg_config --sharedir`
# semodule -u $SHAREDIR/contrib/sepgsql-regtest.pp
# semodule -l
:
sepgsql-regtest 1.03
:
</screen>

5. I'm not too sure about this one, but I think it might be good to
elaborate on what we mean by respecting the system SE-Linux policy.
What kinds of objects do we support checks on? What sorts of checks?
What kind of access can we allow/deny?

I guess these detailed description makes amount of this chapter
disproportionally increase in the future version.
My preference is wikipage to provide this kind of detailed information.

http://wiki.postgresql.org/wiki/SEPostgreSQL

The contents of above wikipage is now obsoleted, because it assumed
SELinux support as a built-in feature. But it is a good time to fix
up the description.

I'd prefer to have it in the actual documentation. I think
SE-PostgreSQL is going to need a lot of documentation. A wiki page
risks getting out of date or having the wrong information for the
version the user has installed. 9.1 may be quite different from 9.2,
for example.

Indeed, wikipage might not be suitable to document for several different
version. OK, I'll try to add description what you suggested above.

Most of what you have here right now describes why you might
want to use this feature, rather than what the feature actually does.
If you want to start by updating the wiki page, that's fine, and may
be an easier way for us to collaborate than doing it by exchanging
patches. But ultimately I think it needs to go in the docs.

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.

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?

Thanks,
--
KaiGai Kohei <kaigai@ak.jp.nec.com>

2011/1/6 KaiGai Kohei <kaigai@ak.jp.nec.com>:

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.

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

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.

I set up a wikipage as a source of *.sgml file.
http://wiki.postgresql.org/wiki/SEPostgreSQL_Documentation

We can fix up cosmetic things later, so should we review the
description of the upcoming official documentation?

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.

I added a mention about installation path in the "Installation"
section, as follows:

The following instruction assumes your installation is under
the /usr/local/pgsql directory, and the database cluster is
in /usr/local/pgsql/data. Substitute your paths appropriately.

It seems to me natural rather than using `pg_config --sharedir`
instead. (we might need to care about installation path of the
pg_config in this case.)

Thanks,
--
KaiGai Kohei <kaigai@ak.jp.nec.com>

2011/1/5 KaiGai Kohei <kaigai@ak.jp.nec.com>:

How about feasibility to merge this 4KL chunks during the rest of
45 days? I think we should decide this general direction at first.

I read through this code tonight and it looks pretty straightforward.
I don't see much reason not to accept this more or less as-is. I'm a
bit suspicious of this line:

{ "translation", SEPG_PROCESS__TRANSITION },

I can't help wondering based on the rest of the table whether you
intend to have the same word on that line twice, but you don't. On a
related note, would it make sense to pare down this table to the
entries that are actually used at the moment? And how about adding a
ProcessUtility_hook to trap evil non-DML statements that some
nefarious user might issues?

One other problem is that you need to work on your whitespace a bit.
I believe in a few places you have a mixture of tabs and spaces. More
seriously, pgindent is going to completely mangle things like this:

/*
* 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 SEPGSQL_MODE_PERMISSIVE,
* except for
no audit prints
*/

You have to write it with a line of dashes on the first and last
lines, if you don't want it reformatted as a paragraph. It might be
worth actually running pgindent over contrib/selinux and then check
over the results.

Finally, we need to work on the documentation.

But overall, it looks pretty good, IMHO.

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

(2011/01/20 12:10), Robert Haas wrote:

2011/1/5 KaiGai Kohei<kaigai@ak.jp.nec.com>:

How about feasibility to merge this 4KL chunks during the rest of
45 days? I think we should decide this general direction at first.

I read through this code tonight and it looks pretty straightforward.
I don't see much reason not to accept this more or less as-is. I'm a
bit suspicious of this line:

{ "translation", SEPG_PROCESS__TRANSITION },

I can't help wondering based on the rest of the table whether you
intend to have the same word on that line twice, but you don't. On a
related note, would it make sense to pare down this table to the
entries that are actually used at the moment?

Sorry for giving you a confusion.
It was my typo, so should be fixed as:
{ "transition", SEPG_PROCESS_TRANSITION },

This permission shall be checked when a security label of client being
switched from X to Y, typically on execution of trusted-procedure.
Also, I missed to check on sepgsql_needs_fmgr_hook(). We don't want to
allow inlining the function on lack of permission.

I'll fix them soon.

And how about adding a
ProcessUtility_hook to trap evil non-DML statements that some
nefarious user might issues?

It seems to me reasonable as long as the number of controlled command
are limited. For example, LOAD command may be a candidate being
controlled without exceptions.
However, it will be a tough work, if the plug-in tries to parse and
analyze supplied utility commands by itself.

One other problem is that you need to work on your whitespace a bit.
I believe in a few places you have a mixture of tabs and spaces. More
seriously, pgindent is going to completely mangle things like this:

/*
* 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 SEPGSQL_MODE_PERMISSIVE,
* except for
no audit prints
*/

You have to write it with a line of dashes on the first and last
lines, if you don't want it reformatted as a paragraph. It might be
worth actually running pgindent over contrib/selinux and then check
over the results.

OK, I'll try to run pgindent to confirm the effect of reformat.

Finally, we need to work on the documentation.

I uploaded my draft here.
http://wiki.postgresql.org/wiki/SEPostgreSQL_Documentation

If reasonable, I'll move them into *.sgml style.

I may want to simplify the step to installation using an installer
script.

But overall, it looks pretty good, IMHO.

Thanks for your reviewing in spite of a large patch.
--
KaiGai Kohei <kaigai@ak.jp.nec.com>

2011/1/19 KaiGai Kohei <kaigai@ak.jp.nec.com>:

 And how about adding a
ProcessUtility_hook to trap evil non-DML statements that some
nefarious user might issues?

It seems to me reasonable as long as the number of controlled command
are limited. For example, LOAD command may be a candidate being
controlled without exceptions.
However, it will be a tough work, if the plug-in tries to parse and
analyze supplied utility commands by itself.

I think the key is to either accept or reject the command based on
very simple criteria - decide based only on the command type, and
ignore its parameters.

I uploaded my draft here.
 http://wiki.postgresql.org/wiki/SEPostgreSQL_Documentation

If reasonable, I'll move them into *.sgml style.

I have yet to review that, but will try to get to it before too much
more time goes by.

I may want to simplify the step to installation using an installer
script.

OK, but let's get this nailed down as soon as possible. Tempus fugit.

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

(2011/01/20 13:01), Robert Haas wrote:

2011/1/19 KaiGai Kohei<kaigai@ak.jp.nec.com>:

And how about adding a
ProcessUtility_hook to trap evil non-DML statements that some
nefarious user might issues?

It seems to me reasonable as long as the number of controlled command
are limited. For example, LOAD command may be a candidate being
controlled without exceptions.
However, it will be a tough work, if the plug-in tries to parse and
analyze supplied utility commands by itself.

I think the key is to either accept or reject the command based on
very simple criteria - decide based only on the command type, and
ignore its parameters.

I can understand this idea, however, it is hard to implement this
criteria, because SELinux describes all the rules as a relationship
between a client and object using their label, so we cannot know
what actions (typically represented in command tag) are allowed or
denied without resolving their object names.

I uploaded my draft here.
http://wiki.postgresql.org/wiki/SEPostgreSQL_Documentation

If reasonable, I'll move them into *.sgml style.

I have yet to review that, but will try to get to it before too much
more time goes by.

OK, I try to translate it into *.sgml format.

I may want to simplify the step to installation using an installer
script.

OK, but let's get this nailed down as soon as possible. Tempus fugit.

I like to give my higher priority on the ProcessUtility_hook, rather
than installation script.

Thanks,
--
KaiGai Kohei <kaigai@ak.jp.nec.com>

Excerpts from Robert Haas's message of jue ene 20 00:10:55 -0300 2011:

You have to write it with a line of dashes on the first and last
lines, if you don't want it reformatted as a paragraph. It might be
worth actually running pgindent over contrib/selinux and then check
over the results.

Hmm, I don't think pgindent is run regularly on contrib as it is on the
core code.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Thu, Jan 20, 2011 at 9:59 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Excerpts from Robert Haas's message of jue ene 20 00:10:55 -0300 2011:

You have to write it with a line of dashes on the first and last
lines, if you don't want it reformatted as a paragraph.  It might be
worth actually running pgindent over contrib/selinux and then check
over the results.

Hmm, I don't think pgindent is run regularly on contrib as it is on the
core code.

I went back and looked at commit
239d769e7e05e0a5ef3bd6828e93e22ef3962780 and it touches both src and
contrib. But even if we don't always do that, it seems like a good
idea to fix whatever we're committing so that a hypothetical future
pgindent run won't mangle it.

Incidentally, I thought that running pgindent twice in the 9.0 cycle,
once after the end of CF4 and again just before the branch worked
well. Anyone up for doing it that way again this time?

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

Alvaro Herrera <alvherre@commandprompt.com> writes:

Hmm, I don't think pgindent is run regularly on contrib as it is on the
core code.

News to me.

regards, tom lane