Re: sepgsql contrib module

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(at)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(at)ak(dot)jp(dot)nec(dot)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(at)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(at)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(at)ak(dot)jp(dot)nec(dot)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(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)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(at)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(at)ak(dot)jp(dot)nec(dot)com>

2011/1/5 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

2011/1/6 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

2011/1/6 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

2011/1/5 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)com>

2011/1/19 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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(at)commandprompt(dot)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

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<kaigai(at)ak(dot)jp(dot)nec(dot)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.
>

--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2011/1/21 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> - Add checks to avoid inlining function without db_procedure:{execute}
>  permission. Sorry, process:{transition} shall be checked in other place.

Hrm. What happens if permissions change between plan time and execution time?

For that matter, I wonder what happens with regular function
permissions. If the plan inlines the function and then somebody goes
and changes the permission on the function and makes it SECURITY
DEFINER, what happens?

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> For that matter, I wonder what happens with regular function
> permissions. If the plan inlines the function and then somebody goes
> and changes the permission on the function and makes it SECURITY
> DEFINER, what happens?

ALTER FUNCTION is supposed to cause plan invalidation in such a case.
Not sure if GRANT plays nice with that though.

regards, tom lane

On Fri, Jan 21, 2011 at 9:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> For that matter, I wonder what happens with regular function
>> permissions.  If the plan inlines the function and then somebody goes
>> and changes the permission on the function and makes it SECURITY
>> DEFINER, what happens?
>
> ALTER FUNCTION is supposed to cause plan invalidation in such a case.
> Not sure if GRANT plays nice with that though.

And in the case of SE-Linux, this could get changed from outside the
database. Not sure how to handle that. I guess we could just never
inline anything, but that might be an overreaction.

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

2011/1/22 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Jan 21, 2011 at 9:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> For that matter, I wonder what happens with regular function
>>> permissions.  If the plan inlines the function and then somebody goes
>>> and changes the permission on the function and makes it SECURITY
>>> DEFINER, what happens?
>>
>> ALTER FUNCTION is supposed to cause plan invalidation in such a case.
>> Not sure if GRANT plays nice with that though.
>
> And in the case of SE-Linux, this could get changed from outside the
> database.  Not sure how to handle that.  I guess we could just never
> inline anything, but that might be an overreaction.
>
We can have two standpoints.

The one is that functions are once allowed to execute on the plan time,
so we don't need to check it on execution time, and inlined.
Thus, the function shall be melted.

The other is that permission checks should be done in execution time,
so we never allows to inline functions anyway.
This attitude is more strict, but mostly overreaction.

In my opinion, the later one is more correct standpoint when we put
the highest priority on security.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Jan 21, 2011 at 9:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> ALTER FUNCTION is supposed to cause plan invalidation in such a case.
>> Not sure if GRANT plays nice with that though.

> And in the case of SE-Linux, this could get changed from outside the
> database. Not sure how to handle that. I guess we could just never
> inline anything, but that might be an overreaction.

I think SELinux is just out of luck in that case. If it didn't refuse
execution permission at the time we checked before inlining (which we
do), it doesn't get to change its mind later.

regards, tom lane

On Fri, Jan 21, 2011 at 10:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Jan 21, 2011 at 9:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> ALTER FUNCTION is supposed to cause plan invalidation in such a case.
>>> Not sure if GRANT plays nice with that though.
>
>> And in the case of SE-Linux, this could get changed from outside the
>> database.  Not sure how to handle that.  I guess we could just never
>> inline anything, but that might be an overreaction.
>
> I think SELinux is just out of luck in that case.  If it didn't refuse
> execution permission at the time we checked before inlining (which we
> do), it doesn't get to change its mind later.

Seems reasonable to me, if it works for KaiGai.

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

2011/1/22 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Jan 21, 2011 at 10:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Fri, Jan 21, 2011 at 9:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> ALTER FUNCTION is supposed to cause plan invalidation in such a case.
>>>> Not sure if GRANT plays nice with that though.
>>
>>> And in the case of SE-Linux, this could get changed from outside the
>>> database.  Not sure how to handle that.  I guess we could just never
>>> inline anything, but that might be an overreaction.
>>
>> I think SELinux is just out of luck in that case.  If it didn't refuse
>> execution permission at the time we checked before inlining (which we
>> do), it doesn't get to change its mind later.
>
> Seems reasonable to me, if it works for KaiGai.
>
I assume users of SE-PostgreSQL put their first priority on security,
not best-performance. So, I also think it is reasonable to kill a part of
optimization for the strict security checks.

Here is one request for the hook.
needs_fmgr_hook() is called by fmgr_info_cxt_security() and routines
to inline. I need a flag to distinct these cases, because we don't need
to invoke all the functions via fmgr_security_definer(), even if it never
allows to inline.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Fri, Jan 21, 2011 at 11:00 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2011/1/22 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Fri, Jan 21, 2011 at 10:46 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> On Fri, Jan 21, 2011 at 9:55 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>>> ALTER FUNCTION is supposed to cause plan invalidation in such a case.
>>>>> Not sure if GRANT plays nice with that though.
>>>
>>>> And in the case of SE-Linux, this could get changed from outside the
>>>> database.  Not sure how to handle that.  I guess we could just never
>>>> inline anything, but that might be an overreaction.
>>>
>>> I think SELinux is just out of luck in that case.  If it didn't refuse
>>> execution permission at the time we checked before inlining (which we
>>> do), it doesn't get to change its mind later.
>>
>> Seems reasonable to me, if it works for KaiGai.
>>
> I assume users of SE-PostgreSQL put their first priority on security,
> not best-performance. So, I also think it is reasonable to kill a part of
> optimization for the strict security checks.
>
> Here is one request for the hook.
> needs_fmgr_hook() is called by fmgr_info_cxt_security() and routines
> to inline. I need a flag to distinct these cases, because we don't need
> to invoke all the functions via fmgr_security_definer(), even if it never
> allows to inline.

I don't want to go there, and it's not what Tom was proposing anyway.
The idea is - if the user creates a function which is NOT a trusted
procedure and executes it, and then subsequently changes the system
security policy so that it becomes a trusted procedure, the user will
be responsible for flushing the cached plans before the new value will
take effect. That doesn't require nearly as much de-optimization, and
I don't believe it is a serious issue from a security perspective,
either. (Note that the reverse case, where a trusted procedure is
demoted to a non-trusted procedure, isn't an issue, because we will
have suppressed inlining and the new execution will follow the right
rules, just with reduced performance.)

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I don't want to go there, and it's not what Tom was proposing anyway.
> The idea is - if the user creates a function which is NOT a trusted
> procedure and executes it, and then subsequently changes the system
> security policy so that it becomes a trusted procedure, the user will
> be responsible for flushing the cached plans before the new value will
> take effect.

Yeah. Given the rather limited set of things that can be inlined,
I don't think that it's worth the complexity or performance cost to
do differently. Note also that it's pretty easy to force the cache
flush if you are the procedure's owner: any sort of dummy ALTER on
the procedure should do it.

Mind you, I think there probably *is* a case for fixing REVOKE to force
a cache flush on the procedure as well. I just don't want to have to
deal with magic outside-the-database changes.

regards, tom lane

2011/1/21 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> Do we have any workaround to avoid these indenting/formatting?
> Or, the reformatted code is better than before?

That's pretty horrendous. Tom/Bruce, any ideas?

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

On Sun, Jan 23, 2011 at 03:19, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 2011/1/21 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> Do we have any workaround to avoid these indenting/formatting?
>> Or, the reformatted code is better than before?
>
> That's pretty horrendous.  Tom/Bruce, any ideas?

I saw some similar things earlier, and it turned out to be two
different reasons in two different cases. In one case, it was because
I was using GNU indent, even though I thought I was using the one
that's on our ftp. But it does give a warning in that case, you just
have to actually *read* the warning. In the other case it was really
weird - when my wrapper script (that called pgindent with path
specification and such) executing using dash (the default /bin/sh on
Ubuntu), it did weird things - but when I explicitly executed the
wrapper script with /bin/bash, it worked - even though pgindent itself
is still using /bin/sh.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Sun, Jan 23, 2011 at 03:19, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> That's pretty horrendous. Tom/Bruce, any ideas?

> I saw some similar things earlier, and it turned out to be two
> different reasons in two different cases. In one case, it was because
> I was using GNU indent, even though I thought I was using the one
> that's on our ftp. But it does give a warning in that case, you just
> have to actually *read* the warning. In the other case it was really
> weird - when my wrapper script (that called pgindent with path
> specification and such) executing using dash (the default /bin/sh on
> Ubuntu), it did weird things - but when I explicitly executed the
> wrapper script with /bin/bash, it worked - even though pgindent itself
> is still using /bin/sh.

Hm, but then the inner /bin/sh is really dash no? Maybe the outer
invocation is setting environment variables or something to change the
behavior of the inner invocation. That would be pretty broken, but IME
most bash substitutes are pretty broken.

regards, tom lane

2011/1/21 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> The attached patch is a revised version.

I've committed this. Cleanup coming...

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

On Sun, Jan 23, 2011 at 8:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> 2011/1/21 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> The attached patch is a revised version.
>
> I've committed this.  Cleanup coming...

Yikes. On further examination, exec_object_restorecon() is pretty
bogus. Surely you need some calls to quote_literal_cstr() in there
someplace. And how about using getObjectDescriptionOids() for the
error message, instead of the entirely bogus construction that's there
now?

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

On Sun, Jan 23, 2011 at 9:56 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sun, Jan 23, 2011 at 8:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> 2011/1/21 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> The attached patch is a revised version.
>>
>> I've committed this.  Cleanup coming...
>
> Yikes.  On further examination, exec_object_restorecon() is pretty
> bogus.  Surely you need some calls to quote_literal_cstr() in there
> someplace.  And how about using getObjectDescriptionOids() for the
> error message, instead of the entirely bogus construction that's there
> now?

Also, shouldn't a bunch of these messages end in ": %m"?

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

(2011/01/24 12:49), Robert Haas wrote:
> On Sun, Jan 23, 2011 at 9:56 PM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>> On Sun, Jan 23, 2011 at 8:53 PM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
>>> 2011/1/21 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>>> The attached patch is a revised version.
>>>
>>> I've committed this. Cleanup coming...
>>
>> 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.

>> And how about using getObjectDescriptionOids() for the
>> error message, instead of the entirely bogus construction that's there
>> now?
>
It seems to me a good idea. I'll try to revise corresponding code.

> Also, shouldn't a bunch of these messages end in ": %m"?
>
When these messages are because of unexpected operating system errors,
such as fails in communication with selinux, the "%m" will give us good
suggestions.

I'll submit these fixes within a few days, please wait for a while.

Thanks,
--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

(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?

Thanks,
--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2011/1/25 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> (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().

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

(2011/01/27 0:25), Robert Haas wrote:
> 2011/1/25 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> (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 <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2011/1/27 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> - 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

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

(2011/01/27 22:26), Robert Haas wrote:
> 2011/1/27 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>> - 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 <kaigai(at)ak(dot)jp(dot)nec(dot)com>

2011/1/27 KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>:
> (2011/01/27 22:26), Robert Haas wrote:
>> 2011/1/27 KaiGai Kohei<kaigai(at)ak(dot)jp(dot)nec(dot)com>:
>>> - 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.

Committed.

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

On Wed, Feb 2, 2011 at 11:43 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Committed.

I did some more polishing of the documentation as well.

It would be good to have some buildfarm coverage of this code. Can we
find anyone brave enough to set up a buildfarm critter using
--with-selinux?

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

Sorry for the late responding, because of my relocation.

> It would be good to have some buildfarm coverage of this code. Can we
> find anyone brave enough to set up a buildfarm critter using
> --with-selinux?
>
Although I don't have an account on the buildfarm, I'll set up an environment
for daily build with --with-selinux.
Because it needs kernel with selinux, libselinux and security policy, I think
it is good idea to set up a virtual machine for the purpose.

Thanks,

> -----Original Message-----
> From: Robert Haas [mailto:robertmhaas(at)gmail(dot)com]
> Sent: 03 February 2011 05:27
> To: KaiGai Kohei
> Cc: KaiGai Kohei; PgHacker
> Subject: Re: [HACKERS] sepgsql contrib module
>
> On Wed, Feb 2, 2011 at 11:43 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > Committed.
>
> I did some more polishing of the documentation as well.
>
> It would be good to have some buildfarm coverage of this code. Can we
> find anyone brave enough to set up a buildfarm critter using
> --with-selinux?
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company

KaiGai,

* Kohei Kaigai (Kohei(dot)Kaigai(at)EU(dot)NEC(dot)COM) wrote:
> > It would be good to have some buildfarm coverage of this code. Can we
> > find anyone brave enough to set up a buildfarm critter using
> > --with-selinux?
> >
> Although I don't have an account on the buildfarm, I'll set up an environment
> for daily build with --with-selinux.
> Because it needs kernel with selinux, libselinux and security policy, I think
> it is good idea to set up a virtual machine for the purpose.

We really need to get a buildfarm which is building with this. To that
end, would you mind providing directions so someone else could set up a
buildfarm member to test it..?

Thanks,

Stephen

> We really need to get a buildfarm which is building with this. To that
> end, would you mind providing directions so someone else could set up a
> buildfarm member to test it..?

It seems to me not difficult to describe a direction to build, install and
run regression test.
Do we have any Fedora 14 environment in the buildfarm?
It is the most suitable distribution to set up sepgsql module, because the
default installation already has selinux configurations.

Thanks,

> -----Original Message-----
> From: Stephen Frost [mailto:sfrost(at)snowman(dot)net]
> Sent: 14 February 2011 16:29
> To: Kohei Kaigai
> Cc: Robert Haas; KaiGai Kohei; PgHacker
> Subject: Re: [HACKERS] sepgsql contrib module
>
> KaiGai,
>
> * Kohei Kaigai (Kohei(dot)Kaigai(at)EU(dot)NEC(dot)COM) wrote:
> > > It would be good to have some buildfarm coverage of this code. Can
> > > we find anyone brave enough to set up a buildfarm critter using
> > > --with-selinux?
> > >
> > Although I don't have an account on the buildfarm, I'll set up an
> > environment for daily build with --with-selinux.
> > Because it needs kernel with selinux, libselinux and security policy,
> > I think it is good idea to set up a virtual machine for the purpose.
>
> We really need to get a buildfarm which is building with this. To that
> end, would you mind providing directions so someone else could set up a
> buildfarm member to test it..?
>
> Thanks,
>
> Stephen

Excerpts from Kohei Kaigai's message of lun feb 14 13:47:58 -0300 2011:
> > We really need to get a buildfarm which is building with this. To that
> > end, would you mind providing directions so someone else could set up a
> > buildfarm member to test it..?
>
> It seems to me not difficult to describe a direction to build, install and
> run regression test.
> Do we have any Fedora 14 environment in the buildfarm?
> It is the most suitable distribution to set up sepgsql module, because the
> default installation already has selinux configurations.

It would be good to fix the Makefile problem that prevents the code to
build elsewhere. There's another thread about a hardcoded path to a
system Makefile in contrib/sepgsql that causes that module to FTBFS. Is
there a way to fix that? I had a look some days ago and it didn't seem
like there was a way to query the system for the proper path to use.

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

On 02/14/2011 11:47 AM, Kohei Kaigai wrote:
>> We really need to get a buildfarm which is building with this. To that
>> end, would you mind providing directions so someone else could set up a
>> buildfarm member to test it..?
> It seems to me not difficult to describe a direction to build, install and
> run regression test.
> Do we have any Fedora 14 environment in the buildfarm?
> It is the most suitable distribution to set up sepgsql module, because the
> default installation already has selinux configurations.

Thew makefile still has this bogosity:

sepgsql-regtest.pp: sepgsql-regtest.te
$(MAKE) -f $(DESTDIR)/usr/share/selinux/devel/Makefile $@

We need to fix that up before we even think of trying to get buildfarm
coverage. The presence and location of this makefile should be
determined at configure time, ISTM.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Thew makefile still has this bogosity:

> sepgsql-regtest.pp: sepgsql-regtest.te
> $(MAKE) -f $(DESTDIR)/usr/share/selinux/devel/Makefile $@

> We need to fix that up before we even think of trying to get buildfarm
> coverage. The presence and location of this makefile should be
> determined at configure time, ISTM.

I'd suggest just getting rid of the $(DESTDIR), which is flat out wrong,
and leaving it as-is otherwise. The portability level of this code is
somewhere at the bad-joke level anyway; if you're trying to get it to
run anywhere but a recent Fedora/RHEL system, I really doubt that path
is the first or biggest problem you'll hit.

regards, tom lane

On 02/14/2011 04:21 PM, Tom Lane wrote:
> Andrew Dunstan<andrew(at)dunslane(dot)net> writes:
>> Thew makefile still has this bogosity:
>> sepgsql-regtest.pp: sepgsql-regtest.te
>> $(MAKE) -f $(DESTDIR)/usr/share/selinux/devel/Makefile $@
>> We need to fix that up before we even think of trying to get buildfarm
>> coverage. The presence and location of this makefile should be
>> determined at configure time, ISTM.
> I'd suggest just getting rid of the $(DESTDIR), which is flat out wrong,
> and leaving it as-is otherwise. The portability level of this code is
> somewhere at the bad-joke level anyway; if you're trying to get it to
> run anywhere but a recent Fedora/RHEL system, I really doubt that path
> is the first or biggest problem you'll hit.

Yeah. The next thing I hit was this:

[andrew(at)aurelia sepgsql]$ make -f /usr/share/selinux/devel/Makefile
sepgsql-regtest.pp
cat: /selinux/mls: No such file or directory
make: *** No rule to make target `sepgsql-regtest.pp'. Stop.
[andrew(at)aurelia sepgsql]$

That's not very nice.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Yeah. The next thing I hit was this:

> [andrew(at)aurelia sepgsql]$ make -f /usr/share/selinux/devel/Makefile
> sepgsql-regtest.pp
> cat: /selinux/mls: No such file or directory
> make: *** No rule to make target `sepgsql-regtest.pp'. Stop.
> [andrew(at)aurelia sepgsql]$

Hmph. A build with --with-selinux goes through for me on a
pretty-vanilla Fedora 13 installation (at least the build and install
steps, I dunno how to test it).

It looks to me like /selinux/mls is some weird phony-filesystem file,
because "cat" prints one character (a "1") while "ls" claims the file is
of zero length. So it's probably something consed up by the kernel,
like /proc/. Do you have selinux enabled on your machine?

(BTW, testing what seems to be a kernel-configuration-reporting flag at
build time strikes me as pretty awful design.)

regards, tom lane

On 02/14/2011 08:36 PM, Tom Lane wrote:
> Andrew Dunstan<andrew(at)dunslane(dot)net> writes:
>> Yeah. The next thing I hit was this:
>> [andrew(at)aurelia sepgsql]$ make -f /usr/share/selinux/devel/Makefile
>> sepgsql-regtest.pp
>> cat: /selinux/mls: No such file or directory
>> make: *** No rule to make target `sepgsql-regtest.pp'. Stop.
>> [andrew(at)aurelia sepgsql]$
> Hmph. A build with --with-selinux goes through for me on a
> pretty-vanilla Fedora 13 installation (at least the build and install
> steps, I dunno how to test it).
>
> It looks to me like /selinux/mls is some weird phony-filesystem file,
> because "cat" prints one character (a "1") while "ls" claims the file is
> of zero length. So it's probably something consed up by the kernel,
> like /proc/. Do you have selinux enabled on your machine?

Np, but that really shouldn't be a build requirement, ISTM, even if it
is a test requirement.

> (BTW, testing what seems to be a kernel-configuration-reporting flag at
> build time strikes me as pretty awful design.)
>
>

Yeah, I agree.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 02/14/2011 08:36 PM, Tom Lane wrote:
>> It looks to me like /selinux/mls is some weird phony-filesystem file,
>> because "cat" prints one character (a "1") while "ls" claims the file is
>> of zero length. So it's probably something consed up by the kernel,
>> like /proc/. Do you have selinux enabled on your machine?

> Np, but that really shouldn't be a build requirement, ISTM, even if it
> is a test requirement.

[ A few reboots later... ] Yeah, I've confirmed that /selinux/mls isn't
there at all when SELinux is disabled. When it is there, it reflects
the setting of SELINUXTYPE ("targeted" or "mls"). So that explains what
/usr/share/selinux/devel/Makefile is doing, but it doesn't make it a
good idea.

>> (BTW, testing what seems to be a kernel-configuration-reporting flag at
>> build time strikes me as pretty awful design.)

> Yeah, I agree.

Yup, this is just broken by design.

It looks like /usr/share/selinux/devel/Makefile basically just sets NAME
and TYPE and then calls /usr/share/selinux/devel/include/Makefile, so we
could avoid the dependence on the build machine's current state if we
did that for ourselves. Of course that just begs the question of what
we should set these variables *to*. Since the file being built is only
used for regression testing, it wouldn't be unreasonable to pick some
values, but it's not clear to me whether things would go blooey if the
eventual test machine had different settings.

On the whole, I don't think that sepgsql-regtest.pp should be built or
installed at all during the build phase. It ought to be generated
during regression test startup, instead.

regards, tom lane

On Mon, Feb 14, 2011 at 9:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> On the whole, I don't think that sepgsql-regtest.pp should be built or
> installed at all during the build phase.  It ought to be generated
> during regression test startup, instead.

You have to manually install and enable it before you can run the
regression tests. This is documented.

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

On 02/15/2011 10:34 AM, Robert Haas wrote:
> On Mon, Feb 14, 2011 at 9:55 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> On the whole, I don't think that sepgsql-regtest.pp should be built or
>> installed at all during the build phase. It ought to be generated
>> during regression test startup, instead.
> You have to manually install and enable it before you can run the
> regression tests. This is documented.
>

That's not the point. Right now you can't even run just a postgresql
build on a machine that doesn't have selinux enabled, let alone run the
regression tests. And if we construct the regression gadget at
postgresql build time, who is to say it has the right settings for the
run time environment, given that the build is apparently dependent on a
runtime kernel setting?

cheers

andrew

On Tue, Feb 15, 2011 at 10:50 AM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> On 02/15/2011 10:34 AM, Robert Haas wrote:
>> On Mon, Feb 14, 2011 at 9:55 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us>  wrote:
>>> On the whole, I don't think that sepgsql-regtest.pp should be built or
>>> installed at all during the build phase.  It ought to be generated
>>> during regression test startup, instead.
>>
>> You have to manually install and enable it before you can run the
>> regression tests.  This is documented.
>
> That's not the point. Right now you can't even run just a postgresql build
> on a machine that doesn't have selinux enabled, let alone run the regression
> tests. And if we construct the regression gadget at postgresql build time,
> who is to say it has the right settings for the run time environment, given
> that the build is apparently dependent on a runtime kernel setting?

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'.

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

Robert Haas <robertmhaas(at)gmail(dot)com> 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.

regards, tom lane

On Tue, Feb 15, 2011 at 11:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> 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.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Feb 15, 2011 at 11:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> 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.

regards, tom lane

On Tue, Feb 15, 2011 at 11:41 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Feb 15, 2011 at 11:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> 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.

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

> -----Original Message-----
> From: Robert Haas [mailto:robertmhaas(at)gmail(dot)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 <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> On Tue, Feb 15, 2011 at 11:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>> Robert Haas <robertmhaas(at)gmail(dot)com> 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 <kohei(dot)kaigai(at)eu(dot)nec(dot)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 <kohei(dot)kaigai(at)eu(dot)nec(dot)com>

> -----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(at)gmail(dot)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 <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > >> On Tue, Feb 15, 2011 at 11:01 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > >>> Robert Haas <robertmhaas(at)gmail(dot)com> 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 <kohei(dot)kaigai(at)eu(dot)nec(dot)com>

On Thu, Feb 17, 2011 at 3:56 AM, Kohei Kaigai <Kohei(dot)Kaigai(at)eu(dot)nec(dot)com> wrote:
> The attached patch removes rules to build a policy package for regression
> test and modifies documentation part to introduce steps to run the test.

Committed. Incidentally, on my Ubuntu system:

$ find /usr/share/selinux -name '*ake*'
/usr/share/selinux/default/include/Makefile
/usr/share/selinux/ubuntu/include/Makefile
/usr/share/selinux/mls/include/Makefile

Not sure which of these would be the right one to use.

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

Sorry so much!
I thought I replied to the question already, but not yet.

> $ find /usr/share/selinux -name '*ake*'
> /usr/share/selinux/default/include/Makefile
> /usr/share/selinux/ubuntu/include/Makefile
> /usr/share/selinux/mls/include/Makefile
>
> Not sure which of these would be the right one to use.
>
The 4th level entry shall be replaced by policy type.

So, if "ubuntu" policy type is available on the system, the Makefile
we shall use is /usr/share/selinux/ubuntu/include/Makefile .
^^^^^^

We can confirm the current available policy type from /etc/selinux/config
or using sestatus command.

[kaigai(at)vmlinux tmp]$ sestatus
SELinux status: enabled
SELinuxfs mount: /selinux
Current mode: enforcing
Mode from config file: enforcing
Policy version: 24
Policy from config file: targeted
^^^^^^^^ It is the policy type.

In this case, the current available policy type is "targeted".

BTW, it seems to me the base version of selinux-policy-* package in Ubuntu
is forked from an older snapshot (20091117), so it does not have enough rules
to run SE-PostgreSQL.

Right now, Fedora 13/14 is the easiest way.

Thanks,
--
NEC Europe Ltd, Global Competence Center
KaiGai Kohei <kohei(dot)kaigai(at)eu(dot)nec(dot)com>

> -----Original Message-----
> From: Robert Haas [mailto:robertmhaas(at)gmail(dot)com]
> Sent: 17. Februar 2011 11:42
> To: Kohei Kaigai
> Cc: Tom Lane; Andrew Dunstan; Stephen Frost; KaiGai Kohei; PgHacker
> Subject: Re: [HACKERS] sepgsql contrib module
>
> On Thu, Feb 17, 2011 at 3:56 AM, Kohei Kaigai <Kohei(dot)Kaigai(at)eu(dot)nec(dot)com>
> wrote:
> > The attached patch removes rules to build a policy package for regression
> > test and modifies documentation part to introduce steps to run the test.
>
> Committed. Incidentally, on my Ubuntu system:
>
> $ find /usr/share/selinux -name '*ake*'
> /usr/share/selinux/default/include/Makefile
> /usr/share/selinux/ubuntu/include/Makefile
> /usr/share/selinux/mls/include/Makefile
>
> Not sure which of these would be the right one to use.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> Click
> https://www.mailcontrol.com/sr/1JPOTPNZc+vTndxI!oX7UnkyRQ0MRq91W9aRlCO
> 56S1wi0rtpLI1rpvj957f8eUOrAhhBS0z5yrieLvRJKIvyA== to report this email
> as spam.

On Thu, Mar 3, 2011 at 5:38 AM, Kohei Kaigai <Kohei(dot)Kaigai(at)eu(dot)nec(dot)com> wrote:
> BTW, it seems to me the base version of selinux-policy-* package in Ubuntu
> is forked from an older snapshot (20091117), so it does not have enough rules
> to run SE-PostgreSQL.
>
> Right now, Fedora 13/14 is the easiest way.

Yeah. I think that pretty much sucks, because really we would like
this to work wherever SE-Linux works. But I suppose in time it will
fix itself.

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