Re: Updates of SE-PostgreSQL 8.4devel patches (r1197)

I updated the patch set of SE-PostgreSQL (revision 1197).

[1/6] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1197.patch
[2/6] http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r1197.patch
[3/6] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1197.patch
[4/6] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1197.patch
[5/6] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1197.patch
[6/6] http://sepgsql.googlecode.com/files/sepostgresql-row_acl-8.4devel-3-r1197.patch

List of updates:
- Patches are rebased to the latest CVS HEAD.
- The "NSA SELinux" comments are replaced by the simple "SELinux".
- bugfix: system attributes are preserved prior to invocation of
before-row-update-triggers.
- Add a documentation of row-level database acl, within 6th patch.
Most of patch size increments come from the new documentation.

Draft of the SE-PostgreSQL documentation is here:
http://wiki.postgresql.org/wiki/SEPostgreSQL

By the way, now a week has gone since beginning of the final CommitFest,
however, the reviewers field on the wiki is empty.
Is there anyone who can help to review the patches?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Fri, 2008-11-07 at 18:20 +0900, KaiGai Kohei wrote:
> I updated the patch set of SE-PostgreSQL (revision 1197).
>
> [1/6] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1197.patch
> [2/6] http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r1197.patch
> [3/6] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1197.patch
> [4/6] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1197.patch
> [5/6] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1197.patch
> [6/6] http://sepgsql.googlecode.com/files/sepostgresql-row_acl-8.4devel-3-r1197.patch
>
> List of updates:
> - Patches are rebased to the latest CVS HEAD.
> - The "NSA SELinux" comments are replaced by the simple "SELinux".
> - bugfix: system attributes are preserved prior to invocation of
> before-row-update-triggers.
> - Add a documentation of row-level database acl, within 6th patch.
> Most of patch size increments come from the new documentation.
>
> Draft of the SE-PostgreSQL documentation is here:
> http://wiki.postgresql.org/wiki/SEPostgreSQL
>
> By the way, now a week has gone since beginning of the final CommitFest,
> however, the reviewers field on the wiki is empty.
> Is there anyone who can help to review the patches?

Some initial thoughts based upon reading the Wiki. I've not been
involved in things up to now, so if this dredges up old discussions,
well, these are my thoughts.

I want SEPostgreSQL, but I'd like it to work without needing to be a
compile time option so people can just use it as/when needed. Plus we
don't want to support what would be/is essentially a fork of Postgres.
Most CPUs will optimise away a simple if-test in various places.

Some users will be able to take advantage of the facilities without
implementing full MLS. Yet we want the full facilities for Government.
Many people currently run multiple customers in different schemas,
though this would let them just run a single set of tables so is much
better for running many small customers.

I'm very unhappy about putting a nonoptional value on tuple headers,
especially because it is updatable. Do we expect MVCC will work with
updatable security contexts? i.e. when you update the security context
of a tuple that won't effect its visibility to existing system users. I
can't imagine you'd want that would you? It's kind of difficult to *not*
get it though.

Looks to me that this feature is useless without things working at row
level. So we can't leave that part out.

The security context on each row could be an optional column present
only if HEAP_HASSECURITYCONTEXT is set (0x0010 see htup.h), just like
OIDs. Use a specific datatype rather than TEXT. That datatype could be
an identifier to pg_security. Security people have big databases too, so
we need to compress the security context more and take out parse time of
string handling. Don't think we should use Oids, they're too big. Might
be easier to use a 2byte field and restrict access to 32,000 contexts,
which is easily enough. TEXT also makes me nervous, just in case there
is some collation/encoding weirdness that allows contexts to be
subverted. Fixed integers are hard to compromise in that respect.

How will unique indexes work? Do you implicitly add security context as
last column on every unique index, or does the uniqueness violation only
occurs within security contexts, or does the uniqueness violation tested
against all contextx that the inserter can currently see? Is there a
change to system catalogs?

Foreign Key deletions could be handled correctly if you treat them as
updates. If we have the following example

TableA
security_context=y value=2 fk=1

TableB
security_context=x value=1

TableA refers to TableB. Context x cannot see context y.

So if somebody with context x tries to delete value1 from TableB, they
will be refused because of a row they cannot see. In this case the
correct action is to update the tuple in TableB so it now has a
security_context = y. The user with x cannot see it and can be persuaded
he deleted it, while the user with y can still see it.

The section on LOAD doesn't sound very convincing. Loading a module
could immediately subvert security. We could probably tighten up on that
for general use as well. ISTM we need something like a new catalog table
for loadable modules, so we can give them specific security contexts and
potentially store some kind of verification information about them.
Having a single "can load modules" isn't enough with Postgres, since it
would effectively prevent us from loading *any* modules in a secure
database. Which kinda removes much of the benefit of using Postgres.

Is there an issue with relation cache and catalog caches? ISTM that you
should put a security context onto each shared invalidation message, so
that backends know not to maintain caches for data they aren't allowed
to see. Probably overkill, just thinking.

The interaction with SELinux should not be hardcoded. I think we need
some form of plugin/wrapper to allow other systems to work. Not sure
what those are, but this isn't a Linux only project. We want to give
everybody the ability to work with PostgreSQL.

How does Discretionary Access control work with regard to server logs?
You said there was no superuser access, but I don't see any controls for
the logs. Do we need log_max_security_context?

Trusted procedures seem very similar to SECURITY DEFINER functions. Can
you explain what the differences are? I'm sure we don't want to similar
features.

I don't see any reason for the "=" in most of the new DDL syntax. Just
seems superfluous and out of character with normal SQL DDL.

With DDL we already have table options, so why not include
security_context as an option? If we are adding this to databases as
well, it seems a good time to include a generic database-level options
facility and make "security_context" the first option.

The parameter to enable this facility should be something like
enhanced_security = on

My feeling is that if you want to include these features in core
PostgreSQL you won't be able to maintain the "separate branding" of
SEPostgreSQL, logo etc.. Maybe I'll get used to it. But if we are going
to use it, we should say SEPostgresSQL, not SE-PostgreSQL if we are
saying SELinux.

It would make sense for you to look at the work on replication also. Not
much use having SEPostgreSQL if we can't replicate it securely. And
possibly in-place update so that respects label security.

I've not looked at the code and probably won't have time to do that. But
if I did, I'd say "diff -c".

Good, thorough work.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> Some initial thoughts based upon reading the Wiki. I've not been
> involved in things up to now, so if this dredges up old discussions,
> well, these are my thoughts.
>
> I want SEPostgreSQL, but I'd like it to work without needing to be a
> compile time option so people can just use it as/when needed. Plus we
> don't want to support what would be/is essentially a fork of Postgres.
> Most CPUs will optimise away a simple if-test in various places.

Well, I assume SEPostgreSQL will rely on libraries that only exist on
SELinux, so I am afraid there will have to be some type of compile-time
flag, though the goal is to have as much of it enabled as possible at
the SQL level, as we have discussed and the patch reflects.

> Some users will be able to take advantage of the facilities without
> implementing full MLS. Yet we want the full facilities for Government.
> Many people currently run multiple customers in different schemas,
> though this would let them just run a single set of tables so is much
> better for running many small customers.
>
> I'm very unhappy about putting a nonoptional value on tuple headers,
> especially because it is updatable. Do we expect MVCC will work with
> updatable security contexts? i.e. when you update the security context
> of a tuple that won't effect its visibility to existing system users. I
> can't imagine you'd want that would you? It's kind of difficult to *not*
> get it though.
>
> Looks to me that this feature is useless without things working at row
> level. So we can't leave that part out.

Having the security column be optional is an interesting idea, perhaps
like we do with oids and with a GUC variable controlling the default.

I think the patch itself is going to go through many adjustments before
application, I bet.

> The security context on each row could be an optional column present
> only if HEAP_HASSECURITYCONTEXT is set (0x0010 see htup.h), just like
> OIDs. Use a specific datatype rather than TEXT. That datatype could be
> an identifier to pg_security. Security people have big databases too, so
> we need to compress the security context more and take out parse time of
> string handling. Don't think we should use Oids, they're too big. Might
> be easier to use a 2byte field and restrict access to 32,000 contexts,
> which is easily enough. TEXT also makes me nervous, just in case there
> is some collation/encoding weirdness that allows contexts to be
> subverted. Fixed integers are hard to compromise in that respect.

I think the security mechanism is more complex than just assigning a
single security identifier, but perhaps not; I am unsure.

> How will unique indexes work? Do you implicitly add security context as
> last column on every unique index, or does the uniqueness violation only
> occurs within security contexts, or does the uniqueness violation tested
> against all contextx that the inserter can currently see? Is there a
> change to system catalogs?
>
> Foreign Key deletions could be handled correctly if you treat them as
> updates. If we have the following example
>
> TableA
> security_context=y value=2 fk=1
>
> TableB
> security_context=x value=1
>
> TableA refers to TableB. Context x cannot see context y.

I think this is the "covert channel" we have been discussing.

> The interaction with SELinux should not be hardcoded. I think we need
> some form of plugin/wrapper to allow other systems to work. Not sure
> what those are, but this isn't a Linux only project. We want to give
> everybody the ability to work with PostgreSQL.

I think that is why the text security column was used, so other systems
could put their own security text in there.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

> Foreign Key deletions could be handled correctly if you treat them as
> updates. If we have the following example
>
> TableA
> security_context=y value=2 fk=1
>
> TableB
> security_context=x value=1
>
> TableA refers to TableB. Context x cannot see context y.
>
> So if somebody with context x tries to delete value1 from TableB, they
> will be refused because of a row they cannot see. In this case the
> correct action is to update the tuple in TableB so it now has a
> security_context = y. The user with x cannot see it and can be persuaded
> he deleted it, while the user with y can still see it.

It seems odd for a low-privilege user to be able to elevate the
privilege of a tuple above their own privilege level. I also don't
believe that the privilege level is a total order, which might make
this something of a sticky wicket. But those are just my thoughts as
a non-guru.

...Robert

On Fri, 2008-11-07 at 13:19 -0500, Bruce Momjian wrote:
> The security context on each row could be an optional column present
> > only if HEAP_HASSECURITYCONTEXT is set (0x0010 see htup.h), just
> like
> > OIDs. Use a specific datatype rather than TEXT. That datatype could
> be
> > an identifier to pg_security. Security people have big databases
> too, so
> > we need to compress the security context more and take out parse
> time of
> > string handling. Don't think we should use Oids, they're too big.
> Might
> > be easier to use a 2byte field and restrict access to 32,000
> contexts,
> > which is easily enough. TEXT also makes me nervous, just in case
> there
> > is some collation/encoding weirdness that allows contexts to be
> > subverted. Fixed integers are hard to compromise in that respect.
>
> I think the security mechanism is more complex than just assigning a
> single security identifier, but perhaps not; I am unsure.

Maybe. We already handle such complexity for comboids and multixacts, so
I suggest we do the same thing here.

Any system with more than 32,000 security contexts is going to be
unmanageable and probably therefore insecure...

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Fri, 2008-11-07 at 15:12 -0500, Robert Haas wrote:
> > Foreign Key deletions could be handled correctly if you treat them as
> > updates. If we have the following example
> >
> > TableA
> > security_context=y value=2 fk=1
> >
> > TableB
> > security_context=x value=1
> >
> > TableA refers to TableB. Context x cannot see context y.
> >
> > So if somebody with context x tries to delete value1 from TableB, they
> > will be refused because of a row they cannot see. In this case the
> > correct action is to update the tuple in TableB so it now has a
> > security_context = y. The user with x cannot see it and can be persuaded
> > he deleted it, while the user with y can still see it.
>
> It seems odd for a low-privilege user to be able to elevate the
> privilege of a tuple above their own privilege level. I also don't
> believe that the privilege level is a total order, which might make
> this something of a sticky wicket. But those are just my thoughts as
> a non-guru.

The low-privilege user isn't elevating the label. If the tuple was
visible by multiple labels it was already elevated. All I am suggesting
is the system remove the one it can see, leaving the other ones intact.
This makes the row appear to be deleted by the lower privileged user,
whereas in fact it was merely updated. There need not be any ordering to
the labels for this scheme to work.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

> The low-privilege user isn't elevating the label. If the tuple was
> visible by multiple labels it was already elevated. All I am suggesting
> is the system remove the one it can see, leaving the other ones intact.
> This makes the row appear to be deleted by the lower privileged user,
> whereas in fact it was merely updated. There need not be any ordering to
> the labels for this scheme to work.

I see. That seems like it makes sense, but what about the update case?

...Robert

On Fri, Nov 07, 2008 at 01:50:18PM +0000, Simon Riggs wrote:
> How will unique indexes work? Do you implicitly add security context as
> last column on every unique index, or does the uniqueness violation only
> occurs within security contexts, or does the uniqueness violation tested
> against all contextx that the inserter can currently see? Is there a
> change to system catalogs?

The wiki clearly states that the unique test is prior to any filtering.
Anything else seems crazy to me.

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

> Foreign Key deletions could be handled correctly if you treat them as
> updates. If we have the following example

Why? If a client does a delete and the database says OK, the tuple
should be gone, *for everyone*.

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

It is the responsibility of the DB administrator to worry about covert
channels.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> Any system with more than 32,000 security contexts is going to be
> unmanageable and probably therefore insecure...

Perhaps, but it's doubtful that such a restriction will actually save
any space after you consider the alignment behavior. I'd go with an
Oid.

regards, tom lane

Simon Riggs wrote:
> > > So if somebody with context x tries to delete value1 from TableB, they
> > > will be refused because of a row they cannot see. In this case the
> > > correct action is to update the tuple in TableB so it now has a
> > > security_context = y. The user with x cannot see it and can be persuaded
> > > he deleted it, while the user with y can still see it.
> >
> > It seems odd for a low-privilege user to be able to elevate the
> > privilege of a tuple above their own privilege level. I also don't
> > believe that the privilege level is a total order, which might make
> > this something of a sticky wicket. But those are just my thoughts as
> > a non-guru.
>
> The low-privilege user isn't elevating the label. If the tuple was
> visible by multiple labels it was already elevated. All I am suggesting
> is the system remove the one it can see, leaving the other ones intact.
> This makes the row appear to be deleted by the lower privileged user,
> whereas in fact it was merely updated. There need not be any ordering to
> the labels for this scheme to work.

Simon, would you read the chapter on "covert channels"? You might
understand it better than I do and it might give you some ideas:

http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.33.5950

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Simon, Thanks for your comments.

> Some initial thoughts based upon reading the Wiki. I've not been
> involved in things up to now, so if this dredges up old discussions,
> well, these are my thoughts.
>
> I want SEPostgreSQL, but I'd like it to work without needing to be a
> compile time option so people can just use it as/when needed. Plus we
> don't want to support what would be/is essentially a fork of Postgres.
> Most CPUs will optimise away a simple if-test in various places.

As Bruce also mentioned, it has to be linked with libselinux to communicate
in-kernel SELinux, but it is not onw of the universal libraries, so, it
has to be enabled/disabled on build-time option.

In addition, we can also disable the feature by the following configuration.
sepostgresql = disabled (at $PGDATA/postgresql.conf )

TODO: add a description about the guc variable. It can have four state:
"default", "enforcing", "permissive" and "disabled".

> Some users will be able to take advantage of the facilities without
> implementing full MLS. Yet we want the full facilities for Government.
> Many people currently run multiple customers in different schemas,
> though this would let them just run a single set of tables so is much
> better for running many small customers.

SELinux community also provides a MLS enabled policy, but it is not
a default one now. SE-PostgreSQL has all the its access controls
decision externally, so it is out of our coltrols.

> I'm very unhappy about putting a nonoptional value on tuple headers,
> especially because it is updatable. Do we expect MVCC will work with
> updatable security contexts? i.e. when you update the security context
> of a tuple that won't effect its visibility to existing system users. I
> can't imagine you'd want that would you? It's kind of difficult to *not*
> get it though.

When a user updates the security context of some tuples, it is invisible
for other clients until its commit, like as other normal data.
Sorry, it is unclear for me what is the concern you mention.

> Looks to me that this feature is useless without things working at row
> level. So we can't leave that part out.

I guess you are saying the core PostgreSQL also has table and column
level granularities, but its criteria to make a decision is different.
SE-PostgreSQL makes its decision based on the privileges of peer process,
not a database role.

> The security context on each row could be an optional column present
> only if HEAP_HASSECURITYCONTEXT is set (0x0010 see htup.h), just like
> OIDs. Use a specific datatype rather than TEXT. That datatype could be
> an identifier to pg_security. Security people have big databases too, so
> we need to compress the security context more and take out parse time of
> string handling. Don't think we should use Oids, they're too big. Might
> be easier to use a 2byte field and restrict access to 32,000 contexts,
> which is easily enough. TEXT also makes me nervous, just in case there
> is some collation/encoding weirdness that allows contexts to be
> subverted. Fixed integers are hard to compromise in that respect.

An issue is who can decide the existance or needs of security system
attribute. If the table owner can disable it, we cannot say it as
a mandatory access control feature, so the security attribute has to
be always appeared when the security feature is enabled.

Here is an answer for the expected question.
When we refer the "security_context" system column of tuples without
HEAP_HAS_SECURITY, it returns an alternative label called as "unlabeled_t",
because it has no labels.

The reason why we adopt TEXT type is SELinux requires userspace
object manager makes queries via text represented security context,
and it also can be used for other security feature to show client
its security attribute in human readable format.

For your information, in-kernel SELinux can handle 2^32 - 1 of security
context internally in theoretical maximum, so using Oid as a security
identifier is a fair decision to avoid an implementation to handle overflow
cases.

> How will unique indexes work? Do you implicitly add security context as
> last column on every unique index, or does the uniqueness violation only
> occurs within security contexts, or does the uniqueness violation tested
> against all contextx that the inserter can currently see? Is there a
> change to system catalogs?

The unique/primary key constraint works at the lowest level.
So, it has a possibility that invisible tuple prevent to insert a tuple.

> Foreign Key deletions could be handled correctly if you treat them as
> updates. If we have the following example
>
> TableA
> security_context=y value=2 fk=1
>
> TableB
> security_context=x value=1
>
> TableA refers to TableB. Context x cannot see context y.
>
> So if somebody with context x tries to delete value1 from TableB, they
> will be refused because of a row they cannot see. In this case the
> correct action is to update the tuple in TableB so it now has a
> security_context = y. The user with x cannot see it and can be persuaded
> he deleted it, while the user with y can still see it.

It is also discussed before.
In this case, SE-PostgreSQL prevent to delete a tuple on the TableB
to keep referential integrity. So, it enables unprivileged users to
infer the existance of refering tuples, but it is a limitation called
as "covert channel".

Your example is the simplest case, so it seems to work well, but
most of cases are not obvious. If the TE policy prevent accesses
on tuples with security_context=x, we have no obvious way to decide
what is a proper security context to be updated.

> The section on LOAD doesn't sound very convincing. Loading a module
> could immediately subvert security. We could probably tighten up on that
> for general use as well. ISTM we need something like a new catalog table
> for loadable modules, so we can give them specific security contexts and
> potentially store some kind of verification information about them.
> Having a single "can load modules" isn't enough with Postgres, since it
> would effectively prevent us from loading *any* modules in a secure
> database. Which kinda removes much of the benefit of using Postgres.

SE-PostgreSQL applies access controls for individual loadable modules.
When a function implemented within an external modules tries to be loaded,
it checks security context between the database and the file of loadable
modules. (No need to say, in-kernel SELinux assigns its security context
for files in common format, so we can compare them each other.)

Perhaps, the description I wrote can easily make misunderstanding.
If you can read it says widespread load modules permission, I'll revise
its representation.

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

> Is there an issue with relation cache and catalog caches? ISTM that you
> should put a security context onto each shared invalidation message, so
> that backends know not to maintain caches for data they aren't allowed
> to see. Probably overkill, just thinking.

Sorry, I cannot understand what you concerned.
The pg_security system catalog is only modified when a newly appeared
text representation of security attribute is given. So, it is read only
for most of cases.

> The interaction with SELinux should not be hardcoded. I think we need
> some form of plugin/wrapper to allow other systems to work. Not sure
> what those are, but this isn't a Linux only project. We want to give
> everybody the ability to work with PostgreSQL.

The PGACE security framework already enables what you want.
The "rowacl" module is a proof of concept, and we can add a new mechanism
on the framework, if necessary.
Do you know the LSM (Linux Security Module) system?

> How does Discretionary Access control work with regard to server logs?
> You said there was no superuser access, but I don't see any controls for
> the logs. Do we need log_max_security_context?

It writes out server logs to filesystem, so its access controls should
be applied in-kernel SELinux. SE-PostgreSQL does not care system call
invocation, because it should be a task in kernel features.
It works as a reference monitor for SQL queries.

> Trusted procedures seem very similar to SECURITY DEFINER functions. Can
> you explain what the differences are? I'm sure we don't want to similar
> features.

It does not change the working security context of the client.

Please consider whether a set-uid program on operating system makes
unnecessary domain transition of SELinux, or not. They have individual
purposes and roles, so we need both of them.

> I don't see any reason for the "=" in most of the new DDL syntax. Just
> seems superfluous and out of character with normal SQL DDL.

Because I thought the syntax is most friendliness for end-users
two years ago. It looks obvious the option specifies the security
context of resources.
Sorry, it is not a clear reason, but it easily makes us to understand
the option.

> With DDL we already have table options, so why not include
> security_context as an option? If we are adding this to databases as
> well, it seems a good time to include a generic database-level options
> facility and make "security_context" the first option.

How do we implement the "security_context" option for columns?
When we want to define a table with explicitly labeled column,
I don't think the table option is a user friendly interface.

> The parameter to enable this facility should be something like
> enhanced_security = on

As I noted above, it already provides:
sepostgresql = [ default | enforcing | permissive | disabled ]

> My feeling is that if you want to include these features in core
> PostgreSQL you won't be able to maintain the "separate branding" of
> SEPostgreSQL, logo etc.. Maybe I'll get used to it. But if we are going
> to use it, we should say SEPostgresSQL, not SE-PostgreSQL if we are
> saying SELinux.

Please fee free to call it as SE-PostgreSQL, SEPostgreSQL, sepgsql
and others. I don't think it is a significant matter.
However, in my hope, I don't want to discard the mascot and logo,
because it was a present from my friend who works as an illustrator.

In Japan, a turtle is used as a mascot of PostgreSQL. :-)
http://www.postgresql.jp/npo/logo/

> It would make sense for you to look at the work on replication also. Not
> much use having SEPostgreSQL if we can't replicate it securely. And
> possibly in-place update so that respects label security.

The log-shipping replication mechanism will works correctly,
but I have not evaluated it yet. In this case, the master
and slaves should have same security policy for databases.

> I've not looked at the code and probably won't have time to do that. But
> if I did, I'd say "diff -c".

OK, I'll fix the scripts to generate patch set.
The next series will provided with "diff -c" style.

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

Simon Riggs wrote:
> On Fri, 2008-11-07 at 15:12 -0500, Robert Haas wrote:
>>> Foreign Key deletions could be handled correctly if you treat them as
>>> updates. If we have the following example
>>>
>>> TableA
>>> security_context=y value=2 fk=1
>>>
>>> TableB
>>> security_context=x value=1
>>>
>>> TableA refers to TableB. Context x cannot see context y.
>>>
>>> So if somebody with context x tries to delete value1 from TableB, they
>>> will be refused because of a row they cannot see. In this case the
>>> correct action is to update the tuple in TableB so it now has a
>>> security_context = y. The user with x cannot see it and can be persuaded
>>> he deleted it, while the user with y can still see it.
>> It seems odd for a low-privilege user to be able to elevate the
>> privilege of a tuple above their own privilege level. I also don't
>> believe that the privilege level is a total order, which might make
>> this something of a sticky wicket. But those are just my thoughts as
>> a non-guru.
>
> The low-privilege user isn't elevating the label. If the tuple was
> visible by multiple labels it was already elevated. All I am suggesting
> is the system remove the one it can see, leaving the other ones intact.
> This makes the row appear to be deleted by the lower privileged user,
> whereas in fact it was merely updated. There need not be any ordering to
> the labels for this scheme to work.

SELinux does not allow a object has two or more labels.

You implicitly assume the security mechanism allows users to access
when one of the labels allows it, but there is no consensus.

At the past, an idea of multiple labels is discussed in SELinux community,
but it was finally dropped because we cannot define the behavior when the
security policy makes different decision for two different labels.

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

On Fri, 2008-11-07 at 16:52 -0500, Bruce Momjian wrote:
> Simon Riggs wrote:
> > > > So if somebody with context x tries to delete value1 from TableB, they
> > > > will be refused because of a row they cannot see. In this case the
> > > > correct action is to update the tuple in TableB so it now has a
> > > > security_context = y. The user with x cannot see it and can be persuaded
> > > > he deleted it, while the user with y can still see it.
> > >
> > > It seems odd for a low-privilege user to be able to elevate the
> > > privilege of a tuple above their own privilege level. I also don't
> > > believe that the privilege level is a total order, which might make
> > > this something of a sticky wicket. But those are just my thoughts as
> > > a non-guru.
> >
> > The low-privilege user isn't elevating the label. If the tuple was
> > visible by multiple labels it was already elevated. All I am suggesting
> > is the system remove the one it can see, leaving the other ones intact.
> > This makes the row appear to be deleted by the lower privileged user,
> > whereas in fact it was merely updated. There need not be any ordering to
> > the labels for this scheme to work.
>
> Simon, would you read the chapter on "covert channels"? You might
> understand it better than I do and it might give you some ideas:
>
> http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.33.5950

It's probably easier just to say "here is the specification we;re
working to implement".

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Sat, 2008-11-08 at 14:21 +0900, KaiGai Kohei wrote:

> > Some users will be able to take advantage of the facilities without
> > implementing full MLS. Yet we want the full facilities for Government.
> > Many people currently run multiple customers in different schemas,
> > though this would let them just run a single set of tables so is much
> > better for running many small customers.
>
> SELinux community also provides a MLS enabled policy, but it is not
> a default one now. SE-PostgreSQL has all the its access controls
> decision externally, so it is out of our coltrols.

I have a couple of concerns:

1. if we make a direct link with SELinux then the code will be *much*
less used and tested. It will be a constant battle to maintain
SEPostgres in a bug-free state. I want to decouple the link so this code
goes into normal Postgres. Other comments below are made with that
thought in mind.

2. I see another use case here. For example, a customer runs a
Software-as-a-service company where the same service is offered to 500
customers. The same database schema is there for each customer, yet they
must never see each other's data. Today, that is implemented as 500
schemas, plus 1 schema with common data in it. ISTM we would be able to
implement this better using SEPostgreSQL, with/without using SELinux.
The need for common data is why in some cases we would want access
controls placed only on certain tables.

Fulfilling use case (2) will also meet my concerns in (1).

So I would prefer it if the solution was designed closely with SELinux,
but usable and useful in other cases also. The link to SELinux could be
done via a contrib plugin. A plugin is then optional. But the main
plugin we provide can be built directly with SELinux.

> > The security context on each row could be an optional column present
> > only if HEAP_HASSECURITYCONTEXT is set (0x0010 see htup.h), just like
> > OIDs. Use a specific datatype rather than TEXT. That datatype could be
> > an identifier to pg_security. Security people have big databases too, so
> > we need to compress the security context more and take out parse time of
> > string handling. Don't think we should use Oids, they're too big. Might
> > be easier to use a 2byte field and restrict access to 32,000 contexts,
> > which is easily enough. TEXT also makes me nervous, just in case there
> > is some collation/encoding weirdness that allows contexts to be
> > subverted. Fixed integers are hard to compromise in that respect.
>
> An issue is who can decide the existance or needs of security system
> attribute. If the table owner can disable it, we cannot say it as
> a mandatory access control feature, so the security attribute has to
> be always appeared when the security feature is enabled.
>
> Here is an answer for the expected question.
> When we refer the "security_context" system column of tuples without
> HEAP_HAS_SECURITY, it returns an alternative label called as "unlabeled_t",
> because it has no labels.

Not really an issue. Just use a parameter. security_controls = mandatory
or change the meaning of the existing one. Similar to default_with_oids,
just that we have a setting where it isn't optional.

> The reason why we adopt TEXT type is SELinux requires userspace
> object manager makes queries via text represented security context,
> and it also can be used for other security feature to show client
> its security attribute in human readable format.

AFAICS, neither of those things means the datatype has to be TEXT.

> For your information, in-kernel SELinux can handle 2^32 - 1 of security
> context internally in theoretical maximum, so using Oid as a security
> identifier is a fair decision to avoid an implementation to handle overflow
> cases.

OK, so we need 4 bytes. I can live with that - at least its not ~10
bytes/row. Does it need to be an Oid, or just the size of an Oid?

> > The section on LOAD doesn't sound very convincing. Loading a module
> > could immediately subvert security. We could probably tighten up on that
> > for general use as well. ISTM we need something like a new catalog table
> > for loadable modules, so we can give them specific security contexts and
> > potentially store some kind of verification information about them.
> > Having a single "can load modules" isn't enough with Postgres, since it
> > would effectively prevent us from loading *any* modules in a secure
> > database. Which kinda removes much of the benefit of using Postgres.
>
> SE-PostgreSQL applies access controls for individual loadable modules.
> When a function implemented within an external modules tries to be loaded,
> it checks security context between the database and the file of loadable
> modules. (No need to say, in-kernel SELinux assigns its security context
> for files in common format, so we can compare them each other.)
>
> Perhaps, the description I wrote can easily make misunderstanding.
> If you can read it says widespread load modules permission, I'll revise
> its representation.
>
> http://wiki.postgresql.org/wiki/SEPostgreSQL#Loading_shared_library_module
>

> > How does Discretionary Access control work with regard to server logs?
> > You said there was no superuser access, but I don't see any controls for
> > the logs. Do we need log_max_security_context?
>
> It writes out server logs to filesystem, so its access controls should
> be applied in-kernel SELinux. SE-PostgreSQL does not care system call
> invocation, because it should be a task in kernel features.
> It works as a reference monitor for SQL queries.

Yes, the file will be protected. My thought was that we might wish to
control the levels of message placed in the file. Otherwise you would
require top level access to view the log. It doesn't seem likely that
they would want to give Top Secret level access to DBAs. Nor should any
level of DBAs be able to inspect who is running which SQL by simple
manipulation of parameters such as log_min_duration_statement.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Fri, 2008-11-07 at 16:52 -0500, Bruce Momjian wrote:
>> Simon Riggs wrote:
>>>>> So if somebody with context x tries to delete value1 from TableB, they
>>>>> will be refused because of a row they cannot see. In this case the
>>>>> correct action is to update the tuple in TableB so it now has a
>>>>> security_context = y. The user with x cannot see it and can be persuaded
>>>>> he deleted it, while the user with y can still see it.
>>>> It seems odd for a low-privilege user to be able to elevate the
>>>> privilege of a tuple above their own privilege level. I also don't
>>>> believe that the privilege level is a total order, which might make
>>>> this something of a sticky wicket. But those are just my thoughts as
>>>> a non-guru.
>>> The low-privilege user isn't elevating the label. If the tuple was
>>> visible by multiple labels it was already elevated. All I am suggesting
>>> is the system remove the one it can see, leaving the other ones intact.
>>> This makes the row appear to be deleted by the lower privileged user,
>>> whereas in fact it was merely updated. There need not be any ordering to
>>> the labels for this scheme to work.
>> Simon, would you read the chapter on "covert channels"? You might
>> understand it better than I do and it might give you some ideas:
>>
>> http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.33.5950
>
> It's probably easier just to say "here is the specification we;re
> working to implement".

This document gives us some of hints to be considered when we
apply mandatory access control facilities on database systems.

However, it is not a specification of SE-PostgreSQL.
The series of documents assumes traditional multi-level-security
system, so it does not care about flexible policy, type-enforcement
rules and collaborating with operating system.

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

On Sat, 2008-11-08 at 18:58 +0900, KaiGai Kohei wrote:

> This document gives us some of hints to be considered when we
> apply mandatory access control facilities on database systems.
>
> However, it is not a specification of SE-PostgreSQL.
> The series of documents assumes traditional multi-level-security
> system, so it does not care about flexible policy, type-enforcement
> rules and collaborating with operating system.

I'm sorry, but I don't understand your answer.

The wiki seemed to indicate, to me, that the FK situation was a problem,
so I was trying to provide a solution. Personally, I could live with it
either way. But the important thing is: will this aspect prevent
SEPostgreSQL from achieving Common Criteria certification, or not?

If it will pass, then I'm happy, even if a different, better solution
exists. If it will fail, then we must act. I'm not qualified to say what
will happen, but it would be good to see a very clear answer on this. If
it was already resolved, then please accept my apologies for raising the
issue again. Please could you update the Wiki docs to explain the agreed
resolution, its reasons and references? The design choices we make will
be questioned again in the future, so it will be good to have them
clear. Thanks.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Sat, 2008-11-08 at 14:21 +0900, KaiGai Kohei wrote:
>
>>> Some users will be able to take advantage of the facilities without
>>> implementing full MLS. Yet we want the full facilities for Government.
>>> Many people currently run multiple customers in different schemas,
>>> though this would let them just run a single set of tables so is much
>>> better for running many small customers.
>> SELinux community also provides a MLS enabled policy, but it is not
>> a default one now. SE-PostgreSQL has all the its access controls
>> decision externally, so it is out of our coltrols.
>
> I have a couple of concerns:
>
> 1. if we make a direct link with SELinux then the code will be *much*
> less used and tested. It will be a constant battle to maintain
> SEPostgres in a bug-free state. I want to decouple the link so this code
> goes into normal Postgres. Other comments below are made with that
> thought in mind.

I don't deny that SELinux has not been our majority yet.
However, I also think other configuratin options have similar issues
less or more. We cannot test all the combinations of the options, so
it should not be a reason.

> 2. I see another use case here. For example, a customer runs a
> Software-as-a-service company where the same service is offered to 500
> customers. The same database schema is there for each customer, yet they
> must never see each other's data. Today, that is implemented as 500
> schemas, plus 1 schema with common data in it. ISTM we would be able to
> implement this better using SEPostgreSQL, with/without using SELinux.
> The need for common data is why in some cases we would want access
> controls placed only on certain tables.
>
> Fulfilling use case (2) will also meet my concerns in (1).

We can assign a proper label for common tables, which allows everyone
to access it. I guess it can be implement wih MCS policy so well.
Any customer has its individual category (c0 to c499) and their data
is also labeled as same category, but the common table is kept
uncategorized.

> So I would prefer it if the solution was designed closely with SELinux,
> but usable and useful in other cases also. The link to SELinux could be
> done via a contrib plugin. A plugin is then optional. But the main
> plugin we provide can be built directly with SELinux.

The reason why SE-PostgreSQL feature is implemented on the PGACE security
framework is to provide end-users options.
We can switch preferable security feature or turn it off at this level.

If necessary, we can implement another option on the PGACE. The rowacl is
proof of the concept. It seems to me you consider a pluggable interface
to operating system enables to apply other security server, but, they can
have different security model, different security attribute and so on.

I think it is an appropriate decision to switch security features at the
hook level which does not provide any security model, more than integration
of different security design.

>>> The security context on each row could be an optional column present
>>> only if HEAP_HASSECURITYCONTEXT is set (0x0010 see htup.h), just like
>>> OIDs. Use a specific datatype rather than TEXT. That datatype could be
>>> an identifier to pg_security. Security people have big databases too, so
>>> we need to compress the security context more and take out parse time of
>>> string handling. Don't think we should use Oids, they're too big. Might
>>> be easier to use a 2byte field and restrict access to 32,000 contexts,
>>> which is easily enough. TEXT also makes me nervous, just in case there
>>> is some collation/encoding weirdness that allows contexts to be
>>> subverted. Fixed integers are hard to compromise in that respect.
>> An issue is who can decide the existance or needs of security system
>> attribute. If the table owner can disable it, we cannot say it as
>> a mandatory access control feature, so the security attribute has to
>> be always appeared when the security feature is enabled.
>>
>> Here is an answer for the expected question.
>> When we refer the "security_context" system column of tuples without
>> HEAP_HAS_SECURITY, it returns an alternative label called as "unlabeled_t",
>> because it has no labels.
>
> Not really an issue. Just use a parameter. security_controls = mandatory
> or change the meaning of the existing one. Similar to default_with_oids,
> just that we have a setting where it isn't optional.

This choice should be contained in the security model which can be choosen
at the PGACE hook level. At least, I cannot imagine SELinux without mandatory
feature. However, I don't deny other security model which allows mandatory or
discretionary.

>> The reason why we adopt TEXT type is SELinux requires userspace
>> object manager makes queries via text represented security context,
>> and it also can be used for other security feature to show client
>> its security attribute in human readable format.
>
> AFAICS, neither of those things means the datatype has to be TEXT.

We cannot represent a security context in TIMESTAMP or FLOAT datatype. :-)

From the viewpoint of user interfaces, I belive TEXT is an appropriate
datatype. It enables to provide an interface to manage security context
as they handle it on operating system.

>> For your information, in-kernel SELinux can handle 2^32 - 1 of security
>> context internally in theoretical maximum, so using Oid as a security
>> identifier is a fair decision to avoid an implementation to handle overflow
>> cases.
>
> OK, so we need 4 bytes. I can live with that - at least its not ~10
> bytes/row. Does it need to be an Oid, or just the size of an Oid?

It utilize the fortune that Oid has 32bit length.
It enables us to indicate a tuple within pg_security without any
additional enhancement.

>>> How does Discretionary Access control work with regard to server logs?
>>> You said there was no superuser access, but I don't see any controls for
>>> the logs. Do we need log_max_security_context?
>> It writes out server logs to filesystem, so its access controls should
>> be applied in-kernel SELinux. SE-PostgreSQL does not care system call
>> invocation, because it should be a task in kernel features.
>> It works as a reference monitor for SQL queries.
>
> Yes, the file will be protected. My thought was that we might wish to
> control the levels of message placed in the file. Otherwise you would
> require top level access to view the log. It doesn't seem likely that
> they would want to give Top Secret level access to DBAs. Nor should any
> level of DBAs be able to inspect who is running which SQL by simple
> manipulation of parameters such as log_min_duration_statement.

In this case, I recommend you to utilize TE rules, not MLS rules.
The default security policy provides postgresql_log_t type which prevent
to manupulate it from others.

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

Simon Riggs wrote:
> On Sat, 2008-11-08 at 18:58 +0900, KaiGai Kohei wrote:
>
>> This document gives us some of hints to be considered when we
>> apply mandatory access control facilities on database systems.
>>
>> However, it is not a specification of SE-PostgreSQL.
>> The series of documents assumes traditional multi-level-security
>> system, so it does not care about flexible policy, type-enforcement
>> rules and collaborating with operating system.
>
> I'm sorry, but I don't understand your answer.

What I wanted to say is that the security design of SELinux is combination
of TE(type enforcement), RBAC(role based access controls) and MLS(multi
level security) so we cannot apply specification of the document as-is.
In addition, its security policy is not hard-wired. These differences
gives us some more technical hurdles.

> The wiki seemed to indicate, to me, that the FK situation was a problem,
> so I was trying to provide a solution. Personally, I could live with it
> either way. But the important thing is: will this aspect prevent
> SEPostgreSQL from achieving Common Criteria certification, or not?

Please note that I've learned the common criteria for a few years but
not a authority, and the answer may depends on certification agency.

In my understanding, it depends on assurance level of the certification
and what functional components are required by the its environment to
be used and threats to be considered here.
If we don't consider who can be a sponsor of the certification, it has
enough functionalities to pass the certification expect for extreme
requirements which well over enterprise class systems.

The covert channel analysis is contained at the FDP_IFF section in the
Common Criteria part 2, and it defines several classes of requirements
in information flow controls.
It defines six components and FDP_IFF.3, 4, 5 mentions the handling of
covert channels, but the 3 and 4 does not require there is no covert
channels.

FYI, some of certified database products also don't mention them.
For example, the certified Oracle Label Security 10g is evaluated as
EAL4+ class, but it mentions only FDP_IFF.2, not 3, 4 and 5.
The FDP_IFF.2 is label based mandatory access controls as SE-PostgreSQL
provides.

> Please could you update the Wiki docs to explain the agreed
> resolution, its reasons and references? The design choices we make will
> be questioned again in the future, so it will be good to have them
> clear. Thanks.

OK, I'll add it to the wiki document.

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

I updated the patch set of SE-PostgreSQL (revision 1206)

[1/6] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1206.patch
[2/6] http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r1206.patch
[3/6] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1206.patch
[4/6] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1206.patch
[5/6] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1206.patch
[6/6] http://sepgsql.googlecode.com/files/sepostgresql-row_acl-8.4devel-3-r1206.patch

Draft of the SE-PostgreSQL documentation is here:
http://wiki.postgresql.org/wiki/SEPostgreSQL

List of updates:
- Patches are rebased to the latest CVS HEAD.
- Style of patches are changes by "diff -c".
- bugfix: {use} permission on user defined function made possibility
to leak information without {select} permission.
- The wikipage is updated as I promised.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Fri, 2008-11-07 at 16:52 -0500, Bruce Momjian wrote:

> Simon, would you read the chapter on "covert channels"? You might
> understand it better than I do and it might give you some ideas:
>
> http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.33.5950

OK, read that now.

Looks to me the covert channel debate will remain open whichever we do.

I agree with you that careful design avoids the problem, for the most
part. Even without that, it appears we have enough to achieve
certification.

The only remaining problem for me now is the size of the security
context column added to each row. I can accept a fixed length 4 byte
value, but anything longer just seems that it will render this unusable.
Normal apps should be able to benefit from row level security, as well
as high-security apps. The additional row overhead is enough to prevent
that, as well as put off many very large high security apps - which is
catastrophic because many of them are very large these days.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> The only remaining problem for me now is the size of the security
> context column added to each row. I can accept a fixed length 4 byte
> value, but anything longer just seems that it will render this unusable.
> Normal apps should be able to benefit from row level security, as well
> as high-security apps. The additional row overhead is enough to prevent
> that, as well as put off many very large high security apps - which is
> catastrophic because many of them are very large these days.

It's unclear for me what is the point you said.
I guess you concern the fixed length field is always allocated in
the case when any security feature is not enabled also, or performance
degradation on the large scale databases.
If incorrect, please tell me in another expression.

At first, the fixed length 4 byte field is allocated only when
the SE-PostgreSQL (or other security feature) is enabled. It can be
controlled via PGACE hook. The pgaceSecurityAttributeNecessary() can
return bool value, and it indicates the necessity of the security
field. If SE-PostgreSQL is disabled on compile-time or run-time,
the fixed length 4 byte value is not allocated.

In the next, we assumes the users of SE-PostgreSQL don't give its
performance the first priority. However, the past measurement shows
its cost is far from the representation of "catastrophic".
The following article shows the result of pgbench at the 8.4devel
with rivision 1063.

http://kaigai.sblo.jp/article/20303941.html

It shows 8% of degradation in the worst cases, and it has a trend
that larger scale database has smaller degradation.
(The measurement is same as I introduced a month ago.)

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

I updated the patch set of SE-PostgreSQL (revision 1216).

There are no new features and bugfixes, but it was rebased to the latest
CVS HEAD, because the previous r1206 got a few conflictions.

These patches are ready for reviewing.
We need your help to make progress them so much!

[1/6] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1216.patch
[2/6] http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r1216.patch
[3/6] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1216.patch
[4/6] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1216.patch
[5/6] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1216.patch
[6/6] http://sepgsql.googlecode.com/files/sepostgresql-row_acl-8.4devel-3-r1216.patch

Draft of the SE-PostgreSQL documentation is here:
http://wiki.postgresql.org/wiki/SEPostgreSQL

I could get a help from a native English speaker who works in SELinux community
as a documentation maintainer, so I'm also updating the description now.
If you notice anything, please feel free to comment also.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Thu, 2008-11-13 at 10:44 +0900, KaiGai Kohei wrote:

> It's unclear for me what is the point you said.
> I guess you concern the fixed length field is always allocated in
> the case when any security feature is not enabled also, or performance
> degradation on the large scale databases.
> If incorrect, please tell me in another expression.
>
> At first, the fixed length 4 byte field is allocated only when
> the SE-PostgreSQL (or other security feature) is enabled. It can be
> controlled via PGACE hook. The pgaceSecurityAttributeNecessary() can
> return bool value, and it indicates the necessity of the security
> field. If SE-PostgreSQL is disabled on compile-time or run-time,
> the fixed length 4 byte value is not allocated.

I'm sorry for not making my thoughts clearer. Let me try again:

As I understand it, when enabled, the overhead for each row is more than
4 bytes because you include a text field also, which you say has a
restricted number of values. IMHO the overhead is unacceptable, given
that our row overhead is already high. I would prefer to make the
maximum overhead per row 4 bytes only, which matches the maximum number
of required labels. This will allow very large databases to use this
feature.

I would also like to see the feature part of normal Postgres, rather
than as a compile time option. The per-row overhead would then be
optional, just as WITH OIDS is optional. This would allow many
applications to take advantage of row level security, without the need
for switching to a different executable and without the need to enable
it for every table. For high security applications, default_row_security
= on would obviously be a requirement. With a single executable on all
distros we will have more robust software and it will be easier to
configure and use.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Thu, 2008-11-13 at 10:44 +0900, KaiGai Kohei wrote:
>
>> It's unclear for me what is the point you said.
>> I guess you concern the fixed length field is always allocated in
>> the case when any security feature is not enabled also, or performance
>> degradation on the large scale databases.
>> If incorrect, please tell me in another expression.
>>
>> At first, the fixed length 4 byte field is allocated only when
>> the SE-PostgreSQL (or other security feature) is enabled. It can be
>> controlled via PGACE hook. The pgaceSecurityAttributeNecessary() can
>> return bool value, and it indicates the necessity of the security
>> field. If SE-PostgreSQL is disabled on compile-time or run-time,
>> the fixed length 4 byte value is not allocated.
>
> I'm sorry for not making my thoughts clearer. Let me try again:
>
> As I understand it, when enabled, the overhead for each row is more than
> 4 bytes because you include a text field also, which you say has a
> restricted number of values. IMHO the overhead is unacceptable, given
> that our row overhead is already high. I would prefer to make the
> maximum overhead per row 4 bytes only, which matches the maximum number
> of required labels. This will allow very large databases to use this
> feature.

Indeed, the additional storage comsumption is not just only fixed 4 bytes
per tuple, because these security identifiers require its text representation.
However, the rate of them are nearly zero in very large databases, especially.

Please consider the following example:

postgres=# SELECT security_context, * FROM t1;
security_context | a | b
---------------------------------------------+---+-----
unconfined_u:object_r:sepgsql_table_t:s0 | 1 | aaa
unconfined_u:object_r:sepgsql_table_t:s0 | 2 | bbb
unconfined_u:object_r:sepgsql_table_t:s0 | 3 | ccc
unconfined_u:object_r:sepgsql_table_t:s0:c0 | 4 | ddd
unconfined_u:object_r:sepgsql_table_t:s0:c0 | 5 | eee
unconfined_u:object_r:sepgsql_table_t:s0:c1 | 6 | fff
unconfined_u:object_r:sepgsql_table_t:s0:c1 | 7 | ggg
unconfined_u:object_r:sepgsql_table_t:s0:c2 | 8 | hhh
(8 rows)

postgres=# SELECT oid, * FROM pg_security;
oid | seclabel
-------+---------------------------------------------
3397 | unconfined_u:object_r:sepgsql_table_t:s0
3398 | unconfined_u:object_r:sepgsql_proc_t:s0
3399 | unconfined_u:object_r:sepgsql_db_t:s0
16390 | unconfined_u:object_r:sepgsql_table_t:s0:c0
16391 | unconfined_u:object_r:sepgsql_table_t:s0:c1
16392 | unconfined_u:object_r:sepgsql_table_t:s0:c2
(6 rows)

The table "t1" contains eight tuples, and there are four kind of security context.
Every tuples has fixed 4-bytes identifier to indicate oid of pg_security which
contains text representation.
(Its output handler translate it into human readable text form.)
The first three tuples have "3397" as its security identifier which indicates
the tuple of "unconfined_u:object_r:sepgsql_table_t:s0" within pg_security.
This entry is shared by them.

If a table has 1,000,000 of tuples labeled by 100 kind of security contexts,
we only need 100 tuples on the pg_security system catalog. In this case, the sum
of text representation length is 5,000 byte at maximum, it is small enough.

In other word, the relationship between a security identifier and an entry of
pg_security is n:1, not 1:1.
Sorry, it seems to me you misunderstand something.

> I would also like to see the feature part of normal Postgres, rather
> than as a compile time option. The per-row overhead would then be
> optional, just as WITH OIDS is optional. This would allow many
> applications to take advantage of row level security, without the need
> for switching to a different executable and without the need to enable
> it for every table. For high security applications, default_row_security
> = on would obviously be a requirement. With a single executable on all
> distros we will have more robust software and it will be easier to
> configure and use.

An issue is who can enable or disable the row-level security option.
If the owner of table can do it discretionary, we don't call it a
"mandatory" access control feature.

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

On Sat, 2008-11-15 at 00:58 +0900, KaiGai Kohei wrote:

> Sorry, it seems to me you misunderstand something.

Yep, seems so. Thank goodness for that. Thanks for putting me straight.

> > I would also like to see the feature part of normal Postgres, rather
> > than as a compile time option. The per-row overhead would then be
> > optional, just as WITH OIDS is optional. This would allow many
> > applications to take advantage of row level security, without the need
> > for switching to a different executable and without the need to enable
> > it for every table. For high security applications, default_row_security
> > = on would obviously be a requirement. With a single executable on all
> > distros we will have more robust software and it will be easier to
> > configure and use.
>
> An issue is who can enable or disable the row-level security option.
> If the owner of table can do it discretionary, we don't call it a
> "mandatory" access control feature.

It seems fairly easy to do that with a GUC, or at least an option on
CREATE DATABASE, with no equivalent ALTER DATABASE option. Once created
with security, a table would not be able to turn off security. So nobody
would be able to turn off security for existing data.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
>>> I would also like to see the feature part of normal Postgres, rather
>>> than as a compile time option. The per-row overhead would then be
>>> optional, just as WITH OIDS is optional. This would allow many
>>> applications to take advantage of row level security, without the need
>>> for switching to a different executable and without the need to enable
>>> it for every table. For high security applications, default_row_security
>>> = on would obviously be a requirement. With a single executable on all
>>> distros we will have more robust software and it will be easier to
>>> configure and use.
>> An issue is who can enable or disable the row-level security option.
>> If the owner of table can do it discretionary, we don't call it a
>> "mandatory" access control feature.
>
> It seems fairly easy to do that with a GUC, or at least an option on
> CREATE DATABASE, with no equivalent ALTER DATABASE option. Once created
> with security, a table would not be able to turn off security. So nobody
> would be able to turn off security for existing data.

SE-PostgreSQL already has a GUC option to enable/disable the feature,
so I don't think a GUC option to control features is not a strange idea.
In fact, I also had considered an option to disable the row-level access
control feature in the similar way, when I got a comment that platform
independent row-level security is a prerequisite to merge SE-PostgreSQL.
However, I made a decision to implement the "rowacl" feature, not a new
GUC option to disable row-level security.

I had two reasons why I didn't implement the option.

The first is the relationship between table/column-level access controls
and row-level controls on system catalogs is unclear.
For example, SE-PostgreSQL handles DELETE on pg_class as an operation to
drop table, because it has same meanings. The table-level access controls
break down, if only row-level checks are disabled.

The other is an issue on implementation. Even if row-level security is
disabled, tuples within pg_class, pg_attribute, pg_proc and pg_largeobject
has to hold its security context, because it means the security context of
tables, columns, procedure and largeobjects.
However, the length of a tuple is computed at heap_form_tuple(), and its
arguments don't contains the object id of relation. Thus, we cannot make
a decision whether the newly created tuple should have a security field, or
not. The heap_form_tuple() is invoked from 60 of functions. It might be
possible to change all the argument list to deliver relation id. But it
seems to me excess updates for the purpose.

Thus, I recommend you to assign uniformed security context whole of the table.
The default security policy allows all the clients to operate anything on
tuples labeled as "sepgsql_table_t" except for relabeling them.
(BTW, it is a default type when here is no special configuration.)

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

On Sun, 2008-11-16 at 01:40 +0900, KaiGai Kohei wrote:

> I had two reasons why I didn't implement the option.
>
> The first is the relationship between table/column-level access controls
> and row-level controls on system catalogs is unclear.
> For example, SE-PostgreSQL handles DELETE on pg_class as an operation to
> drop table, because it has same meanings. The table-level access controls
> break down, if only row-level checks are disabled.

If pg_class needs a special case, no problem.

> The other is an issue on implementation. Even if row-level security is
> disabled, tuples within pg_class, pg_attribute, pg_proc and pg_largeobject
> has to hold its security context, because it means the security context of
> tables, columns, procedure and largeobjects.
> However, the length of a tuple is computed at heap_form_tuple(), and its
> arguments don't contains the object id of relation. Thus, we cannot make
> a decision whether the newly created tuple should have a security field, or
> not. The heap_form_tuple() is invoked from 60 of functions. It might be
> possible to change all the argument list to deliver relation id. But it
> seems to me excess updates for the purpose.

WITH OIDS seems to work well without problem. Why is this different?

I'm sure the problem is solvable. If you just formed the tuple you
probably know enough about a table to get security context. If not,
perhaps security context would be assigned/checked at a lower level.

It was designed with the security community in mind, but it would be
great if it can work for more people. Please could you look to see if
this is possible?

I think it will benefit everybody if this feature can work as an option
of the main server. Currently, this feature seems to support only one
configuration: a special build for SELinux on Red Hat. That's nice, but
I want to see the patch open things up more for other distros/OS, plus
options for people who don't want MAC, but do want row level security.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
>> The other is an issue on implementation. Even if row-level security is
>> disabled, tuples within pg_class, pg_attribute, pg_proc and pg_largeobject
>> has to hold its security context, because it means the security context of
>> tables, columns, procedure and largeobjects.
>> However, the length of a tuple is computed at heap_form_tuple(), and its
>> arguments don't contains the object id of relation. Thus, we cannot make
>> a decision whether the newly created tuple should have a security field, or
>> not. The heap_form_tuple() is invoked from 60 of functions. It might be
>> possible to change all the argument list to deliver relation id. But it
>> seems to me excess updates for the purpose.
>
> WITH OIDS seems to work well without problem. Why is this different?

It is not a problem, but things which take a decision.

The heap_form_tuple() makes a new tuple according to the given
TupleDesc which has a bool variable of "tdhasoid".
This structure can be initialized at various functions. For example,
47 of functions invoke CreateTemplateTupleDesc() which requires an
argument of "bool hasoid".
If we follow the way of WITH OIDS, it will be necessary to modify
the declaration and 47 of invocations, at least.

I don't think your suggestion is a bad idea, except for the number
of modification on the core implementation.

> I think it will benefit everybody if this feature can work as an option
> of the main server. Currently, this feature seems to support only one
> configuration: a special build for SELinux on Red Hat. That's nice, but
> I want to see the patch open things up more for other distros/OS, plus
> options for people who don't want MAC, but do want row level security.

This feature is not limited to Red Hat and related.

Some of distributions now provides SELinux option, but not a default.
I know Debian, Ubuntu, Gentoo and SuSE are doing.
Their default security policies are designed based on the upstreamed
SELinux policy which already has rules for SE-PostgreSQL. So, it will
be delivered to other distributions and enlarge its supporting platforms.

For other operating systems, I know Trusted Solaris folks have a plan
to implement their facilities on the PGACE security framework, at least.

My 6th patch (rowacl) provides a DAC based row-level security implementation.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> Simon Riggs wrote:
>>> The other is an issue on implementation. Even if row-level security is
>>> disabled, tuples within pg_class, pg_attribute, pg_proc and pg_largeobject
>>> has to hold its security context, because it means the security context of
>>> tables, columns, procedure and largeobjects.
>>> However, the length of a tuple is computed at heap_form_tuple(), and its
>>> arguments don't contains the object id of relation. Thus, we cannot make
>>> a decision whether the newly created tuple should have a security field, or
>>> not. The heap_form_tuple() is invoked from 60 of functions. It might be
>>> possible to change all the argument list to deliver relation id. But it
>>> seems to me excess updates for the purpose.
>> WITH OIDS seems to work well without problem. Why is this different?
>
> It is not a problem, but things which take a decision.
>
> The heap_form_tuple() makes a new tuple according to the given
> TupleDesc which has a bool variable of "tdhasoid".
> This structure can be initialized at various functions. For example,
> 47 of functions invoke CreateTemplateTupleDesc() which requires an
> argument of "bool hasoid".
> If we follow the way of WITH OIDS, it will be necessary to modify
> the declaration and 47 of invocations, at least.
>
> I don't think your suggestion is a bad idea, except for the number
> of modification on the core implementation.

In addition, I want you to make clear the priority of the facility.

We can disable row-level access control factually with uniformed
security context which allows client any kind of accesses.
In this case, all tuples shares a single text representation,
so the size of external text representation is enough small to
ignore.

I guess you concerned the per-tuple security attribute because
it has 1:1 mapped text representation on pg_security.
However, it is incorrect.
I don't think we should give high priority for the feature to
kill per-tuple security attribute now.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Tue, 2008-11-18 at 11:51 +0900, KaiGai Kohei wrote:
> Simon Riggs wrote:
> >> The other is an issue on implementation. Even if row-level security is
> >> disabled, tuples within pg_class, pg_attribute, pg_proc and pg_largeobject
> >> has to hold its security context, because it means the security context of
> >> tables, columns, procedure and largeobjects.
> >> However, the length of a tuple is computed at heap_form_tuple(), and its
> >> arguments don't contains the object id of relation. Thus, we cannot make
> >> a decision whether the newly created tuple should have a security field, or
> >> not. The heap_form_tuple() is invoked from 60 of functions. It might be
> >> possible to change all the argument list to deliver relation id. But it
> >> seems to me excess updates for the purpose.
> >
> > WITH OIDS seems to work well without problem. Why is this different?
>
> It is not a problem, but things which take a decision.
>
> The heap_form_tuple() makes a new tuple according to the given
> TupleDesc which has a bool variable of "tdhasoid".
> This structure can be initialized at various functions. For example,
> 47 of functions invoke CreateTemplateTupleDesc() which requires an
> argument of "bool hasoid".
> If we follow the way of WITH OIDS, it will be necessary to modify
> the declaration and 47 of invocations, at least.
>
> I don't think your suggestion is a bad idea, except for the number
> of modification on the core implementation.

We manage to include a NULL bitmap without doing this.

The hasoids option seems to be unused on most of those call points. How
many of those call points would need security context?

The change you suggest looks possible, so maybe it is the way, but it
also looks fairly ugly already. If we did this it would not be by adding
another bool, but by adding an options object.

Another way would be to include a security context in all newly created
tuples, but remove it during heap_update, heap_insert etc if it is
unused by the relation. That seems more straightforward.

> > I think it will benefit everybody if this feature can work as an option
> > of the main server. Currently, this feature seems to support only one
> > configuration: a special build for SELinux on Red Hat. That's nice, but
> > I want to see the patch open things up more for other distros/OS, plus
> > options for people who don't want MAC, but do want row level security.
>
> This feature is not limited to Red Hat and related.
>
> Some of distributions now provides SELinux option, but not a default.
> I know Debian, Ubuntu, Gentoo and SuSE are doing.

SUSE?

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

On Tue, 2008-11-18 at 13:10 +0900, KaiGai Kohei wrote:
> KaiGai Kohei wrote:
> > Simon Riggs wrote:
> >>> The other is an issue on implementation. Even if row-level security is
> >>> disabled, tuples within pg_class, pg_attribute, pg_proc and pg_largeobject
> >>> has to hold its security context, because it means the security context of
> >>> tables, columns, procedure and largeobjects.
> >>> However, the length of a tuple is computed at heap_form_tuple(), and its
> >>> arguments don't contains the object id of relation. Thus, we cannot make
> >>> a decision whether the newly created tuple should have a security field, or
> >>> not. The heap_form_tuple() is invoked from 60 of functions. It might be
> >>> possible to change all the argument list to deliver relation id. But it
> >>> seems to me excess updates for the purpose.
> >> WITH OIDS seems to work well without problem. Why is this different?
> >
> > It is not a problem, but things which take a decision.
> >
> > The heap_form_tuple() makes a new tuple according to the given
> > TupleDesc which has a bool variable of "tdhasoid".
> > This structure can be initialized at various functions. For example,
> > 47 of functions invoke CreateTemplateTupleDesc() which requires an
> > argument of "bool hasoid".
> > If we follow the way of WITH OIDS, it will be necessary to modify
> > the declaration and 47 of invocations, at least.
> >
> > I don't think your suggestion is a bad idea, except for the number
> > of modification on the core implementation.
>
> In addition, I want you to make clear the priority of the facility.
>
> We can disable row-level access control factually with uniformed
> security context which allows client any kind of accesses.
> In this case, all tuples shares a single text representation,
> so the size of external text representation is enough small to
> ignore.

I don't think it is small enough to ignore.

> I guess you concerned the per-tuple security attribute because
> it has 1:1 mapped text representation on pg_security.
> However, it is incorrect.
> I don't think we should give high priority for the feature to
> kill per-tuple security attribute now.

*Maybe* if you view my request this way:

If a table is defined such that all of its tuples have an identical
security-context then we can optimise away the additional field when
storing tuples.

That might help, it might not. Just trying to help us think of different
ways to allow this within the main server variant.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Tue, 2008-11-18 at 11:51 +0900, KaiGai Kohei wrote:
>> Simon Riggs wrote:
>>>> The other is an issue on implementation. Even if row-level security is
>>>> disabled, tuples within pg_class, pg_attribute, pg_proc and pg_largeobject
>>>> has to hold its security context, because it means the security context of
>>>> tables, columns, procedure and largeobjects.
>>>> However, the length of a tuple is computed at heap_form_tuple(), and its
>>>> arguments don't contains the object id of relation. Thus, we cannot make
>>>> a decision whether the newly created tuple should have a security field, or
>>>> not. The heap_form_tuple() is invoked from 60 of functions. It might be
>>>> possible to change all the argument list to deliver relation id. But it
>>>> seems to me excess updates for the purpose.
>>> WITH OIDS seems to work well without problem. Why is this different?
>> It is not a problem, but things which take a decision.
>>
>> The heap_form_tuple() makes a new tuple according to the given
>> TupleDesc which has a bool variable of "tdhasoid".
>> This structure can be initialized at various functions. For example,
>> 47 of functions invoke CreateTemplateTupleDesc() which requires an
>> argument of "bool hasoid".
>> If we follow the way of WITH OIDS, it will be necessary to modify
>> the declaration and 47 of invocations, at least.
>>
>> I don't think your suggestion is a bad idea, except for the number
>> of modification on the core implementation.
>
> We manage to include a NULL bitmap without doing this.

The heap_form_tuple() has an argument of "bool *isnull".
It is a hint of NULL bitmap.

> The hasoids option seems to be unused on most of those call points. How
> many of those call points would need security context?

If we focus on the CreateTemplateTupleDesc(), 5 of call points give
possibile "hasoid" argument, and rest of them always give "false".
I guess it will be same in the security context cases.
However, we have to change all the call points when the declaration
is changed.

> Another way would be to include a security context in all newly created
> tuples, but remove it during heap_update, heap_insert etc if it is
> unused by the relation. That seems more straightforward.

It is not a reasonable option.

The length of HeapTupleData is determined during heap_form_tuple(),
and it is unchanged later. Thus, we have to interpose here, as object
identifier doing.

>> Some of distributions now provides SELinux option, but not a default.
>> I know Debian, Ubuntu, Gentoo and SuSE are doing.
>
> SUSE?

The "u" might be a large-letter.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Tue, 2008-11-18 at 15:02 +0900, KaiGai Kohei wrote:

> If we focus on the CreateTemplateTupleDesc(), 5 of call points give
> possibile "hasoid" argument, and rest of them always give "false".
> I guess it will be same in the security context cases.
> However, we have to change all the call points when the declaration
> is changed.

Looks promising.

> > Another way would be to include a security context in all newly
> created
> > tuples, but remove it during heap_update, heap_insert etc if it is
> > unused by the relation. That seems more straightforward.
>
> It is not a reasonable option.
>
> The length of HeapTupleData is determined during heap_form_tuple(),
> and it is unchanged later. Thus, we have to interpose here, as object
> identifier doing.

Currently yes. Is there a reason not to? Do we rely on the tuple length
staying same after those operations?

Just considering multiple ways of making the context optional.

> >> Some of distributions now provides SELinux option, but not a
> default.
> >> I know Debian, Ubuntu, Gentoo and SuSE are doing.
> >
> > SUSE?
>
> The "u" might be a large-letter.

Sorry, I wasn't correcting your spelling! :-)
I was asking whether Su/USE are definitely supporting SELinux now? I
have not heard that.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
>>> Another way would be to include a security context in all newly
>> created
>>> tuples, but remove it during heap_update, heap_insert etc if it is
>>> unused by the relation. That seems more straightforward.
>> It is not a reasonable option.
>>
>> The length of HeapTupleData is determined during heap_form_tuple(),
>> and it is unchanged later. Thus, we have to interpose here, as object
>> identifier doing.
>
> Currently yes. Is there a reason not to? Do we rely on the tuple length
> staying same after those operations?
>
> Just considering multiple ways of making the context optional.

Indeed, we can consider several options.

However, I don't want to change existing semantics in the core implementation
as far as possible. If we have to choose one of them, I prefer to add TupleDesc
a bool variable to show necessity of security field, because it requires many
points to be updated, but most of them are obvious.

Anyway, I've started to work with the prior approach.
Now we have less than two weeks remained in the CommitFest:Nov, so we have
no time to be spent uselessly.

>>> SUSE?
>> The "u" might be a large-letter.
>
> Sorry, I wasn't correcting your spelling! :-)
> I was asking whether Su/USE are definitely supporting SELinux now? I
> have not heard that.

It is a recent news.
http://news.opensuse.org/2008/08/20/opensuse-to-add-selinux-basic-enablement-in-111/

The openSUSE pressed they start to support SELinux, not only AppArmor.
However, I don't have enough information for the roadmap of SUSE Enterprise Server
which is a production version of Novell.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

>>> The length of HeapTupleData is determined during heap_form_tuple(),
>>> and it is unchanged later. Thus, we have to interpose here, as object
>>> identifier doing.
>> Currently yes. Is there a reason not to? Do we rely on the tuple length
>> staying same after those operations?
>>
>> Just considering multiple ways of making the context optional.
>
> Indeed, we can consider several options.
>
> However, I don't want to change existing semantics in the core implementation
> as far as possible. If we have to choose one of them, I prefer to add TupleDesc
> a bool variable to show necessity of security field, because it requires many
> points to be updated, but most of them are obvious.

Ah, however, it made an explosion of the number of points to be patched soon.

I'll try your approash in first, as follows:

1. When the SECURITY_SYSATTR_NAME is defined, heap_form_tuple() always allocate
a field to store security attribute.
2. The security mechanism can store its security attribute on the hooks for
insert or update tuples. It also can remain the field as InvalidOid.
3. The modified heap_insert() and heap_update() cut off the security field
and moves backward region by sizeof(Oid) bytes, when it is remained as
InvalidOid.

> Anyway, I've started to work with the prior approach.
> Now we have less than two weeks remained in the CommitFest:Nov, so we have
> no time to be spent uselessly.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Tue, 2008-11-18 at 16:51 +0900, KaiGai Kohei wrote:

> Anyway, I've started to work with the prior approach.

Sounds good.

> Now we have less than two weeks remained in the CommitFest:Nov, so we have
> no time to be spent uselessly.
>
> >>> SUSE?
> >> The "u" might be a large-letter.
> >
> > Sorry, I wasn't correcting your spelling! :-)
> > I was asking whether Su/USE are definitely supporting SELinux now? I
> > have not heard that.
>
> It is a recent news.
> http://news.opensuse.org/2008/08/20/opensuse-to-add-selinux-basic-enablement-in-111/
>
> The openSUSE pressed they start to support SELinux, not only AppArmor.
> However, I don't have enough information for the roadmap of SUSE Enterprise Server
> which is a production version of Novell.

OK, I heard that slightly differently. Thanks for the link.
The report I read emphasised the lack of support.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Tue, 2008-11-18 at 16:51 +0900, KaiGai Kohei wrote:
>
>> Anyway, I've started to work with the prior approach.
>
> Sounds good.

The matters currently I faces:

* In the approach.1 (add "tdhassecurity" to TupleDesc)
An explosion of the number of points to be patched. :(

* In the approach.2 (strip security field in heap_insert/update, if unused)

Some implementations assumes an older and newer tuple have
same length of its header, However, a security field can be
striped in the heap_insert/update(), if unused.
Thus, this approach has to allow them to have different length
between older tuple and a result of heap_form_tuple().

In this approach, we have to list up all the implementations
which assume fixed length tuple-header, and fix them.

----
My preference is second one.
It will also enable to clean up many of "hasoid" bool abound the implementation.
However, I want to challenge the enhancement at the v8.5 development cycle,
as a part of writable "oid" system column feature.
(I have a plan to propose the feature next to the SE-PostgreSQL.)

A concern is why you suggested this feature at the last half of the November. :(
*Now*, I don't want to merge the feature to disable row-level security into the
current patch set, because it damages qualities of the codes.

Is there any other opinions?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

On Tue, 2008-11-18 at 21:40 +0900, KaiGai Kohei wrote:

> A concern is why you suggested this feature at the last half of the
> November. :(

I gave you my feedback as soon as I read the Wiki article. Even then I
didn't understand some aspects of the patch design, which was
unfortunate. But these things take time.

I would encourage all developers to post a design description of what
their patch does long before they write it. That way this kind of
feedback comes as early as possible.

Please don't worry. You've done a great job and many people would like
to see your work succeed. Getting things right takes time and it really
is worth it in the end.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> I'll try your approash in first, as follows:

This seems a vast amount of uglification to avoid adding an argument to
CreateTemplateTupleDesc. We do that kind of thing all the time --- it
is a simple and reliable change to make.

When designing a patch, you should generally try to make the code look
like the patch has been there all along. Contorting logic to avoid
a simple API change is not good.

regards, tom lane

Tom Lane wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>> I'll try your approash in first, as follows:
>
> This seems a vast amount of uglification to avoid adding an argument to
> CreateTemplateTupleDesc. We do that kind of thing all the time --- it
> is a simple and reliable change to make.
>
> When designing a patch, you should generally try to make the code look
> like the patch has been there all along. Contorting logic to avoid
> a simple API change is not good.

Hmm..., I think your opinion is right, indeed.

In my conclusion, I want to postpone the feature to toggle row-level
access controls due to its scale of changes.

I can understand the requirements for reduction of storage consumption
with security attribute. I'll implement it next to the main feature of
SE-PostgreSQL.

Simon,
I'm sorry, if my expression felt you uncomfortable.

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

On Wed, 2008-11-19 at 01:42 +0900, KaiGai Kohei wrote:
> Simon,

> I'm sorry, if my expression felt you uncomfortable.

No worries at all. I know exactly how you feel. Good Luck.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Tom Lane wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> > I'll try your approash in first, as follows:
>
> This seems a vast amount of uglification to avoid adding an argument to
> CreateTemplateTupleDesc. We do that kind of thing all the time --- it
> is a simple and reliable change to make.
>
> When designing a patch, you should generally try to make the code look
> like the patch has been there all along. Contorting logic to avoid
> a simple API change is not good.

Just to chime in, I agree with Simon's direction of making the security
specification for the table match WITH OIDS, and agree with Tom that the
implementation should follow the WITH OIDS API for clarity, not trying
to reduce the change footprint. Basically, if WITH OIDS and security
definer behave the same in the API, there is little additional code
_complexity_, even if the patch is now larger.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Tom Lane wrote:
>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>> I'll try your approash in first, as follows:
>> This seems a vast amount of uglification to avoid adding an argument to
>> CreateTemplateTupleDesc. We do that kind of thing all the time --- it
>> is a simple and reliable change to make.
>>
>> When designing a patch, you should generally try to make the code look
>> like the patch has been there all along. Contorting logic to avoid
>> a simple API change is not good.
>
> Just to chime in, I agree with Simon's direction of making the security
> specification for the table match WITH OIDS, and agree with Tom that the
> implementation should follow the WITH OIDS API for clarity, not trying
> to reduce the change footprint. Basically, if WITH OIDS and security
> definer behave the same in the API, there is little additional code
> _complexity_, even if the patch is now larger.

OK, I'll try to start implementing the feature again.

However, the toggle of row-level security feature should be controled
via a GUC option, not a discretionary option.
I'll add a "sepostgresql_row_level" option defined as bool to control
it on start up time.

In addition, please do not stop reviewing the current patch set
due to lack of the feature to disable row-level security.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> Bruce Momjian wrote:
> > Tom Lane wrote:
> >> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> >>> I'll try your approash in first, as follows:
> >> This seems a vast amount of uglification to avoid adding an argument to
> >> CreateTemplateTupleDesc. We do that kind of thing all the time --- it
> >> is a simple and reliable change to make.
> >>
> >> When designing a patch, you should generally try to make the code look
> >> like the patch has been there all along. Contorting logic to avoid
> >> a simple API change is not good.
> >
> > Just to chime in, I agree with Simon's direction of making the security
> > specification for the table match WITH OIDS, and agree with Tom that the
> > implementation should follow the WITH OIDS API for clarity, not trying
> > to reduce the change footprint. Basically, if WITH OIDS and security
> > definer behave the same in the API, there is little additional code
> > _complexity_, even if the patch is now larger.
>
> OK, I'll try to start implementing the feature again.
>
> However, the toggle of row-level security feature should be controled
> via a GUC option, not a discretionary option.
> I'll add a "sepostgresql_row_level" option defined as bool to control
> it on start up time.

This sounds similar to BSD capability were certain security settings can
only be changed in single-user mode.

> In addition, please do not stop reviewing the current patch set
> due to lack of the feature to disable row-level security.

Of course.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> > However, the toggle of row-level security feature should be controled
> > via a GUC option, not a discretionary option.
> > I'll add a "sepostgresql_row_level" option defined as bool to control
> > it on start up time.
>
> This sounds similar to BSD capability were certain security settings can
> only be changed in single-user mode.

Actually, an interesting idea would be to allow "sepostgresql_row_level"
to be turned on, but not off. That means if it was turned on in
postgresql.conf, it could not be turned off, but if it is off in
postgresql.conf, it could be turned on via SET or via ALTER
USER/DATABASE; I think that would be a nice capability.

On a related note, KaiGai, you are now starting the long road of getting
feedback with the ultimate goal of getting your patch into CVS. I will
warn you that there is often much work during this stage, and it might
stretch into January as we request adjustments, but ultimately your
feature and Postgres will be better for it. Thanks for sticking with
it.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Bruce Momjian wrote:
>>> However, the toggle of row-level security feature should be controled
>>> via a GUC option, not a discretionary option.
>>> I'll add a "sepostgresql_row_level" option defined as bool to control
>>> it on start up time.
>> This sounds similar to BSD capability were certain security settings can
>> only be changed in single-user mode.
>
> Actually, an interesting idea would be to allow "sepostgresql_row_level"
> to be turned on, but not off. That means if it was turned on in
> postgresql.conf, it could not be turned off, but if it is off in
> postgresql.conf, it could be turned on via SET or via ALTER
> USER/DATABASE; I think that would be a nice capability.

I think the "sepostgresql_mode" and "sepostgresql_row_level" should not
be toggled frequently.

Please consider SELinux/SE-PostgreSQL requires various kind of objects
(including database objects) to be labeled properly at the initial state.
If it allows clients to turn on row-level security feature, it means many
"unlabeled" tuples appear suddenly. In generally, these have to be labeled
before the system get being available.

> On a related note, KaiGai, you are now starting the long road of getting
> feedback with the ultimate goal of getting your patch into CVS. I will
> warn you that there is often much work during this stage, and it might
> stretch into January as we request adjustments, but ultimately your
> feature and Postgres will be better for it. Thanks for sticking with
> it.

Don't worry, I'm be available for the works, and give a lot for inclusion
of the feature at v8.4.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> Bruce Momjian wrote:
> > Bruce Momjian wrote:
> >>> However, the toggle of row-level security feature should be controled
> >>> via a GUC option, not a discretionary option.
> >>> I'll add a "sepostgresql_row_level" option defined as bool to control
> >>> it on start up time.
> >> This sounds similar to BSD capability were certain security settings can
> >> only be changed in single-user mode.
> >
> > Actually, an interesting idea would be to allow "sepostgresql_row_level"
> > to be turned on, but not off. That means if it was turned on in
> > postgresql.conf, it could not be turned off, but if it is off in
> > postgresql.conf, it could be turned on via SET or via ALTER
> > USER/DATABASE; I think that would be a nice capability.
>
> I think the "sepostgresql_mode" and "sepostgresql_row_level" should not
> be toggled frequently.
>
> Please consider SELinux/SE-PostgreSQL requires various kind of objects
> (including database objects) to be labeled properly at the initial state.
> If it allows clients to turn on row-level security feature, it means many
> "unlabeled" tuples appear suddenly. In generally, these have to be labeled
> before the system get being available.

I was thinking more about people are using the SQL-level row
permissions. Are they going to want it for all tables for all
databases, or not at all? I assumed they would want to be more
selective. I agree the SE-Linux users would probably not toggle it for
different objects and databases frequently.

Actually, you called it "sepostgresql_mode", SE*, but how are we going
to control the SQL-level row permissions? Are we using this name? I
didn't think so because it seems so associated withe SE-Linux, and if
not, how would one say whether a table has SQL-level row permissions?

> > On a related note, KaiGai, you are now starting the long road of getting
> > feedback with the ultimate goal of getting your patch into CVS. I will
> > warn you that there is often much work during this stage, and it might
> > stretch into January as we request adjustments, but ultimately your
> > feature and Postgres will be better for it. Thanks for sticking with
> > it.
>
> Don't worry, I'm be available for the works, and give a lot for inclusion
> of the feature at v8.4.

Great. Sometimes these bold additions can require perhaps thirty
changes before they are added to CVS, I am afraid to say. It can be a
painful part of open source development, even though in the end it is
worth it. (While it is happening, it doesn't seem very useful.)

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

>> Please consider SELinux/SE-PostgreSQL requires various kind of objects
>> (including database objects) to be labeled properly at the initial state.
>> If it allows clients to turn on row-level security feature, it means many
>> "unlabeled" tuples appear suddenly. In generally, these have to be labeled
>> before the system get being available.
>
> I was thinking more about people are using the SQL-level row
> permissions. Are they going to want it for all tables for all
> databases, or not at all?

We don't have a reason why the SQL-level row permissions should be toggled
in global only. It it designed based on DAC policy, so it is quite natural
to be controled by owner of resources.
(However, it is not implemented yet.)

> Actually, you called it "sepostgresql_mode", SE*, but how are we going
> to control the SQL-level row permissions? Are we using this name? I
> didn't think so because it seems so associated withe SE-Linux, and if
> not, how would one say whether a table has SQL-level row permissions?

A new GUC variable of "row_acl_is_enabled" allows us to toggle the SQL-level
row permission feature, when the binary is built with "--enable-row-acl".
In this case, the "sepostgresql_*" are enclosed by #ifdef HAVE_SELINUX, so
these are not available.

>>> On a related note, KaiGai, you are now starting the long road of getting
>>> feedback with the ultimate goal of getting your patch into CVS. I will
>>> warn you that there is often much work during this stage, and it might
>>> stretch into January as we request adjustments, but ultimately your
>>> feature and Postgres will be better for it. Thanks for sticking with
>>> it.
>> Don't worry, I'm be available for the works, and give a lot for inclusion
>> of the feature at v8.4.
>
> Great. Sometimes these bold additions can require perhaps thirty
> changes before they are added to CVS, I am afraid to say. It can be a
> painful part of open source development, even though in the end it is
> worth it. (While it is happening, it doesn't seem very useful.)

s/painful/dynamic/g :-)

If you can ready to post some of comments, earlier is happier for me.
It is unclear for me when the CommtiFest:Nov is finished, but it is sure
we don't have enough days.

In my guess, I'll be able to complete to put a flag within TupleDesc to
control security field of HeapTupleHeader by tomorrow. And I have a plan
to check its behavior in this weekend before merge them into my tree.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> It is unclear for me when the CommtiFest:Nov is finished, but it is sure
> we don't have enough days.

This commitfest goes until it's done. I knew at the beginning that we'd
not be done at the end of November. The original schedule allowed two
months for this fest; we'll see whether that was optimistic or not.

regards, tom lane

KaiGai Kohei wrote:
> >> Please consider SELinux/SE-PostgreSQL requires various kind of objects
> >> (including database objects) to be labeled properly at the initial state.
> >> If it allows clients to turn on row-level security feature, it means many
> >> "unlabeled" tuples appear suddenly. In generally, these have to be labeled
> >> before the system get being available.
> >
> > I was thinking more about people are using the SQL-level row
> > permissions. Are they going to want it for all tables for all
> > databases, or not at all?
>
> We don't have a reason why the SQL-level row permissions should be toggled
> in global only. It it designed based on DAC policy, so it is quite natural
> to be controled by owner of resources.
> (However, it is not implemented yet.)

Yes, that was my question --- how will SQL-level row permissions be
controlled by the user.

> > Actually, you called it "sepostgresql_mode", SE*, but how are we going
> > to control the SQL-level row permissions? Are we using this name? I
> > didn't think so because it seems so associated withe SE-Linux, and if
> > not, how would one say whether a table has SQL-level row permissions?
>
> A new GUC variable of "row_acl_is_enabled" allows us to toggle the SQL-level
> row permission feature, when the binary is built with "--enable-row-acl".

I assumed we would always enable SQL-level row permissions. Why would
it be a compile-time option?

> In this case, the "sepostgresql_*" are enclosed by #ifdef HAVE_SELINUX, so
> these are not available.

Right.

> >>> On a related note, KaiGai, you are now starting the long road of getting
> >>> feedback with the ultimate goal of getting your patch into CVS. I will
> >>> warn you that there is often much work during this stage, and it might
> >>> stretch into January as we request adjustments, but ultimately your
> >>> feature and Postgres will be better for it. Thanks for sticking with
> >>> it.
> >> Don't worry, I'm be available for the works, and give a lot for inclusion
> >> of the feature at v8.4.
> >
> > Great. Sometimes these bold additions can require perhaps thirty
> > changes before they are added to CVS, I am afraid to say. It can be a
> > painful part of open source development, even though in the end it is
> > worth it. (While it is happening, it doesn't seem very useful.)
>
> s/painful/dynamic/g :-)

That's looking on the bright side of things. :-)

> If you can ready to post some of comments, earlier is happier for me.

My guess is we will continue to send you ideas.

> It is unclear for me when the CommtiFest:Nov is finished, but it is sure
> we don't have enough days.

This is the last commit fest for 8.4 so it will probably go well into
December, perhaps January.

> In my guess, I'll be able to complete to put a flag within TupleDesc to
> control security field of HeapTupleHeader by tomorrow. And I have a plan
> to check its behavior in this weekend before merge them into my tree.

Nice. I will look over your newest version as soon as I can.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> KaiGai Kohei wrote:
>>>> Please consider SELinux/SE-PostgreSQL requires various kind of objects
>>>> (including database objects) to be labeled properly at the initial state.
>>>> If it allows clients to turn on row-level security feature, it means many
>>>> "unlabeled" tuples appear suddenly. In generally, these have to be labeled
>>>> before the system get being available.
>>> I was thinking more about people are using the SQL-level row
>>> permissions. Are they going to want it for all tables for all
>>> databases, or not at all?
>> We don't have a reason why the SQL-level row permissions should be toggled
>> in global only. It it designed based on DAC policy, so it is quite natural
>> to be controled by owner of resources.
>> (However, it is not implemented yet.)
>
> Yes, that was my question --- how will SQL-level row permissions be
> controlled by the user.

When the given tuple has no ACL, it applies the default behavior which
allows anything for public. This behavior intends to keep compatible
works compared to the vanilla PostgreSQL.

We can have two state for "no ACLs". The first one is when tuples don't
have fixed 4-bytes security attributes. It can be happen when PostgreSQL
with SQL-level row-permissions mount compatible database files created
by vanilla PostgreSQL. The other is the fixed 4-byte security attribute
indicates an entry of "no ACLs". It seems unnecessary consumption, but
we cannot determine whether the user tries to set ACLs when heap_form_tuple().

One considerable solution is to add an RowACL specific table option to
disable row-level ACLs whole of the tables. It can be suitable for security
design because the option is provided by the resource owner.

For example:

CRATE TABLE t (
a int,
b text
) WITH (row_acl=disable);

If the reloptions contains an information to determine whether a new tuple
need the security field, or not, we can reflect it at heap_form_tuple().

>>> Actually, you called it "sepostgresql_mode", SE*, but how are we going
>>> to control the SQL-level row permissions? Are we using this name? I
>>> didn't think so because it seems so associated withe SE-Linux, and if
>>> not, how would one say whether a table has SQL-level row permissions?
>> A new GUC variable of "row_acl_is_enabled" allows us to toggle the SQL-level
>> row permission feature, when the binary is built with "--enable-row-acl".
>
> I assumed we would always enable SQL-level row permissions. Why would
> it be a compile-time option?

The SQL-level row permission feature is implemented as a guest of PGACE
security framework, so we need to select one of the guest mehanism when
compile-time.
The "--enable-selinux" and "--enable-row-acl" are exclusive option.

>> In my guess, I'll be able to complete to put a flag within TupleDesc to
>> control security field of HeapTupleHeader by tomorrow. And I have a plan
>> to check its behavior in this weekend before merge them into my tree.
>
> Nice. I will look over your newest version as soon as I can.

Today, I could complete to implement the newer version which passes
standard regression tests except for two cases expected.

Simon,
In the result of "pgbench -i -s 10", the "sepostgresql_row_level=true"
case consumed 152MB of storage, and the "sepostgresql_row_level=false"
case consumed 148MB of storage.

[kaigai(at)saba sepgsql]$ du -hs $PGDATA/base/16384
152M /opt/sepgsql/base/16384
[kaigai(at)saba sepgsql]$ du -hs $PGDATA/base/16385
148M /opt/sepgsql/base/16385

Now I'm under preparation to submit the newest patch set.
ToDo:
- Check behavior for SE-PostgreSQL specific operations.
(like security context updates)
- Implementation of new table options to disable row-acl.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> Bruce Momjian wrote:
> > KaiGai Kohei wrote:
> >>>> Please consider SELinux/SE-PostgreSQL requires various kind of objects
> >>>> (including database objects) to be labeled properly at the initial state.
> >>>> If it allows clients to turn on row-level security feature, it means many
> >>>> "unlabeled" tuples appear suddenly. In generally, these have to be labeled
> >>>> before the system get being available.
> >>> I was thinking more about people are using the SQL-level row
> >>> permissions. Are they going to want it for all tables for all
> >>> databases, or not at all?
> >> We don't have a reason why the SQL-level row permissions should be toggled
> >> in global only. It it designed based on DAC policy, so it is quite natural
> >> to be controled by owner of resources.
> >> (However, it is not implemented yet.)
> >
> > Yes, that was my question --- how will SQL-level row permissions be
> > controlled by the user.
>
> When the given tuple has no ACL, it applies the default behavior which
> allows anything for public. This behavior intends to keep compatible
> works compared to the vanilla PostgreSQL.
>
> We can have two state for "no ACLs". The first one is when tuples don't
> have fixed 4-bytes security attributes. It can be happen when PostgreSQL
> with SQL-level row-permissions mount compatible database files created
> by vanilla PostgreSQL. The other is the fixed 4-byte security attribute
> indicates an entry of "no ACLs". It seems unnecessary consumption, but
> we cannot determine whether the user tries to set ACLs when heap_form_tuple().
>
> One considerable solution is to add an RowACL specific table option to
> disable row-level ACLs whole of the tables. It can be suitable for security
> design because the option is provided by the resource owner.
>
> For example:
>
> CRATE TABLE t (
> a int,
> b text
> ) WITH (row_acl=disable);
>
> If the reloptions contains an information to determine whether a new tuple
> need the security field, or not, we can reflect it at heap_form_tuple().

What I am saying is for the default compile, SQL-level ACLs should be
possible because, since the ACL field has optional storage, there is no
downside to have it be available by default.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> KaiGai Kohei wrote:
>> Bruce Momjian wrote:
>>> KaiGai Kohei wrote:
>>>>>> Please consider SELinux/SE-PostgreSQL requires various kind of objects
>>>>>> (including database objects) to be labeled properly at the initial state.
>>>>>> If it allows clients to turn on row-level security feature, it means many
>>>>>> "unlabeled" tuples appear suddenly. In generally, these have to be labeled
>>>>>> before the system get being available.
>>>>> I was thinking more about people are using the SQL-level row
>>>>> permissions. Are they going to want it for all tables for all
>>>>> databases, or not at all?
>>>> We don't have a reason why the SQL-level row permissions should be toggled
>>>> in global only. It it designed based on DAC policy, so it is quite natural
>>>> to be controled by owner of resources.
>>>> (However, it is not implemented yet.)
>>> Yes, that was my question --- how will SQL-level row permissions be
>>> controlled by the user.
>> When the given tuple has no ACL, it applies the default behavior which
>> allows anything for public. This behavior intends to keep compatible
>> works compared to the vanilla PostgreSQL.
>>
>> We can have two state for "no ACLs". The first one is when tuples don't
>> have fixed 4-bytes security attributes. It can be happen when PostgreSQL
>> with SQL-level row-permissions mount compatible database files created
>> by vanilla PostgreSQL. The other is the fixed 4-byte security attribute
>> indicates an entry of "no ACLs". It seems unnecessary consumption, but
>> we cannot determine whether the user tries to set ACLs when heap_form_tuple().
>>
>> One considerable solution is to add an RowACL specific table option to
>> disable row-level ACLs whole of the tables. It can be suitable for security
>> design because the option is provided by the resource owner.
>>
>> For example:
>>
>> CRATE TABLE t (
>> a int,
>> b text
>> ) WITH (row_acl=disable);
>>
>> If the reloptions contains an information to determine whether a new tuple
>> need the security field, or not, we can reflect it at heap_form_tuple().
>
> What I am saying is for the default compile, SQL-level ACLs should be
> possible because, since the ACL field has optional storage, there is no
> downside to have it be available by default.

I think it is a possible and desirable desicion from the viewpoint of
security folks.

However, I think we have a few issues, and it makes unclear whether
we can make an agreement in the community.
The one is a cost of security hooks. They consume a bit more CPU steps
when a security mechanism is enabled. The other is prevention to override
a few hooks (ExecutorRun_hook and planner_hook), because they assume
standard implementations to be executed.

Which is more desirable option in the default?

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

On Fri, 2008-11-21 at 19:47 +0900, KaiGai Kohei wrote:

> In the result of "pgbench -i -s 10", the "sepostgresql_row_level=true"
> case consumed 152MB of storage, and the "sepostgresql_row_level=false"
> case consumed 148MB of storage.

Sounds good. It may not sound much to you, but it is all good news.

This allows us to include the feature in the main release package, which
was my main objective.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

KaiGai Kohei wrote:
> > What I am saying is for the default compile, SQL-level ACLs should be
> > possible because, since the ACL field has optional storage, there is no
> > downside to have it be available by default.
>
> I think it is a possible and desirable desicion from the viewpoint of
> security folks.
>
> However, I think we have a few issues, and it makes unclear whether
> we can make an agreement in the community.
> The one is a cost of security hooks. They consume a bit more CPU steps
> when a security mechanism is enabled. The other is prevention to override
> a few hooks (ExecutorRun_hook and planner_hook), because they assume
> standard implementations to be executed.
>
> Which is more desirable option in the default?

Well, my assumption is that if a table doesn't have SQL-level row
permissions then there is no overhead becaues there are no permissions
to check. If it does have SQL-level row permissions then the user who
created the table has accepted the performance impact of SQL-level row
permissions. I would think it would be pretty easy to see quickly if
any table in a query has SQL-level row permissions and then take the
performance hit.

For example, I might want to put SQL-level row permissions on an audit
table, but none of my other tables, and in that case I assume there is
only a performance impact on queries that use the audit table.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> However, I think we have a few issues, and it makes unclear whether
> we can make an agreement in the community.
> The one is a cost of security hooks. They consume a bit more CPU steps
> when a security mechanism is enabled. The other is prevention to override
> a few hooks (ExecutorRun_hook and planner_hook), because they assume
> standard implementations to be executed.

I think your chances of taking those hooks away are zero. It would
cripple a lot of other facilities that people are more interested in
than they are in SEPostgres. In any case, the only way to use those
hooks is to load C code into the backend, and anyone who can do that
already has the keys to the kingdom. I hope you are not suffering
from any illusions about being able to defend against arbitrary add-on
C code.

regards, tom lane

I updated the patch set of SE-PostgreSQL (revision 1244).

[1/6] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1244.patch
[2/6] http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r1244.patch
[3/6] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1244.patch
[4/6] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1244.patch
[5/6] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1244.patch
[6/6] http://sepgsql.googlecode.com/files/sepostgresql-row_acl-8.4devel-3-r1244.patch

Draft of the SE-PostgreSQL documentation is here:
http://wiki.postgresql.org/wiki/SEPostgreSQL

This revision contains some fixes required by some persons.
(Thanks for Simon, Bruce and Tom.)

List of updates:
- Rebase to the latest CVS HEAD.

- The fixed length security field of HeapTupleHeader becomes optimal.
It enables enhanced security mechanism to control its allocation on
heap_form_tuple(), and to reduce unnecessary storage consumption.
The TupleDesc structure got a new variable of "tdhassecurity".
When it is true, heap_form_tuple() allocates an additional field
to store security identifier. The enhanced security mechanism can
control value of the flag via a new hook: pgaceTupleDescHasSecurity().

- SE-PostgreSQL got a new GUC variable: "sepostgresql_row_level".
When it turned off, SE-PostgreSQL does not apply its row-level
access controls, and does not assign per-tuple security context.

- The following two hooks are removed:
* pgaceIsAllowPlannerHook()
* pgaceIsAllowExecutorRunHook()
And, the following hook is added
* pgaceGramRelationOption()
This hook gives a chance to handle relation options.

- The row-level acl got two new relation options:
* row_level_acl=on|off
When it is tuened off, the row-level access controls are
not applied, and security field is not allocated.
* default_row_acl='...'
It enables to specify a default for newly inserted tuples.

- pg_security system catalog is added to the regression test of
sanity_check.

- Code cleanups related to module installation checks.

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

Tom Lane wrote:
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> However, I think we have a few issues, and it makes unclear whether
>> we can make an agreement in the community.
>> The one is a cost of security hooks. They consume a bit more CPU steps
>> when a security mechanism is enabled. The other is prevention to override
>> a few hooks (ExecutorRun_hook and planner_hook), because they assume
>> standard implementations to be executed.
>
> I think your chances of taking those hooks away are zero. It would
> cripple a lot of other facilities that people are more interested in
> than they are in SEPostgres. In any case, the only way to use those
> hooks is to load C code into the backend, and anyone who can do that
> already has the keys to the kingdom. I hope you are not suffering
> from any illusions about being able to defend against arbitrary add-on
> C code.

I removed the two hooks at the r1244 patch set.
As you said, it is fundamentally danger to load uncertain binary modules.
Thus, what we should do is checks on module loading.

The default security policy requires loadable modules to be labeled as
'lib_t' type which means shared library files installed correctly.

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

Bruce Momjian wrote:
> KaiGai Kohei wrote:
>>> What I am saying is for the default compile, SQL-level ACLs should be
>>> possible because, since the ACL field has optional storage, there is no
>>> downside to have it be available by default.
>> I think it is a possible and desirable desicion from the viewpoint of
>> security folks.
>>
>> However, I think we have a few issues, and it makes unclear whether
>> we can make an agreement in the community.
>> The one is a cost of security hooks. They consume a bit more CPU steps
>> when a security mechanism is enabled. The other is prevention to override
>> a few hooks (ExecutorRun_hook and planner_hook), because they assume
>> standard implementations to be executed.
>>
>> Which is more desirable option in the default?
>
> Well, my assumption is that if a table doesn't have SQL-level row
> permissions then there is no overhead becaues there are no permissions
> to check.

When the binary is built with the SQL-level row permissions and scanned
table does not activated it, all it does is checking a flag variable
at Relation->rd_options. I guess it will be acceptable cost.

In this case, DBA disables row-level permission on the table, so
no additional security field is required.

> For example, I might want to put SQL-level row permissions on an audit
> table, but none of my other tables, and in that case I assume there is
> only a performance impact on queries that use the audit table.

It is not a zero, but tiny as far as we can ignore it in my opinion.

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

On Mon, 2008-11-24 at 22:09 +0900, KaiGai Kohei wrote:

> I removed the two hooks at the r1244 patch set.
> As you said, it is fundamentally danger to load uncertain binary modules.
> Thus, what we should do is checks on module loading.
>
> The default security policy requires loadable modules to be labeled as
> 'lib_t' type which means shared library files installed correctly.

We definitely want to include add-in modules with high security systems,
e.g. GIS and oracle compatibility functions.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Mon, 2008-11-24 at 22:09 +0900, KaiGai Kohei wrote:
>
>> I removed the two hooks at the r1244 patch set.
>> As you said, it is fundamentally danger to load uncertain binary modules.
>> Thus, what we should do is checks on module loading.
>>
>> The default security policy requires loadable modules to be labeled as
>> 'lib_t' type which means shared library files installed correctly.
>
> We definitely want to include add-in modules with high security systems,
> e.g. GIS and oracle compatibility functions.

Yes, it is possible.
SELinux assigns 'lib_t' type for modules stored in '/usr/lib/pgsql/' in default.

like:
[kaigai(at)saba ~]$ ls -Z /usr/lib/pgsql
-rwxr-xr-x root root system_u:object_r:lib_t ascii_and_mic.so
-rwxr-xr-x root root system_u:object_r:lib_t cyrillic_and_mic.so
-rwxr-xr-x root root system_u:object_r:lib_t dict_snowball.so
-rwxr-xr-x root root system_u:object_r:lib_t euc_cn_and_mic.so
-rwxr-xr-x root root system_u:object_r:lib_t euc_jis_2004_and_shift_jis_2004.so
-rwxr-xr-x root root system_u:object_r:lib_t euc_jp_and_sjis.so
-rwxr-xr-x root root system_u:object_r:lib_t euc_kr_and_mic.so
- snip -
(*) "-Z" option enables to show the security context of files.

SE-PostgreSQL does not prevent to load them. It means we want to allow to load library
files stored by database administrators properly, not a uncertain files.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> Bruce Momjian wrote:
> > KaiGai Kohei wrote:
> >>> What I am saying is for the default compile, SQL-level ACLs should be
> >>> possible because, since the ACL field has optional storage, there is no
> >>> downside to have it be available by default.
> >> I think it is a possible and desirable desicion from the viewpoint of
> >> security folks.
> >>
> >> However, I think we have a few issues, and it makes unclear whether
> >> we can make an agreement in the community.
> >> The one is a cost of security hooks. They consume a bit more CPU steps
> >> when a security mechanism is enabled. The other is prevention to override
> >> a few hooks (ExecutorRun_hook and planner_hook), because they assume
> >> standard implementations to be executed.
> >>
> >> Which is more desirable option in the default?
> >
> > Well, my assumption is that if a table doesn't have SQL-level row
> > permissions then there is no overhead becaues there are no permissions
> > to check.
>
> When the binary is built with the SQL-level row permissions and scanned
> table does not activated it, all it does is checking a flag variable
> at Relation->rd_options. I guess it will be acceptable cost.
>
> In this case, DBA disables row-level permission on the table, so
> no additional security field is required.
>
> > For example, I might want to put SQL-level row permissions on an audit
> > table, but none of my other tables, and in that case I assume there is
> > only a performance impact on queries that use the audit table.
>
> It is not a zero, but tiny as far as we can ignore it in my opinion.

Yes. It is good to have SQL-level row security available by default in
every binary, even if it requires a few checks in C.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

I updated the patch set of SE-PostgreSQL (revision 1268).

[1/6] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1268.patch
[2/6] http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r1268.patch
[3/6] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1268.patch
[4/6] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1268.patch
[5/6] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1268.patch
[6/6] http://sepgsql.googlecode.com/files/sepostgresql-row_acl-8.4devel-3-r1268.patch

Draft of the SE-PostgreSQL documentation is here:
http://wiki.postgresql.org/wiki/SEPostgreSQL

List of updates:
- The patches are rebased to the CVS HEAD.
- RelOptions related hooks are cleaned up.
- The Row-level ACL feature is chosen in default.
- rowacl_table_default() is added to show the default ACL of the table.
- The initial revision of regression test for Row-level ACL is added.

If you have anything to comment for the patches, could you disclose it?
It is not necessary to be a comprehensive one. Don't hesitate to submit.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> I updated the patch set of SE-PostgreSQL (revision 1268).
>
> [1/6] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1268.patch
> [2/6] http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r1268.patch
> [3/6] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1268.patch
> [4/6] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1268.patch
> [5/6] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1268.patch
> [6/6] http://sepgsql.googlecode.com/files/sepostgresql-row_acl-8.4devel-3-r1268.patch
>
> Draft of the SE-PostgreSQL documentation is here:
> http://wiki.postgresql.org/wiki/SEPostgreSQL
>
> List of updates:
> - The patches are rebased to the CVS HEAD.
> - RelOptions related hooks are cleaned up.
> - The Row-level ACL feature is chosen in default.
> - rowacl_table_default() is added to show the default ACL of the table.
> - The initial revision of regression test for Row-level ACL is added.
>
> If you have anything to comment for the patches, could you disclose it?
> It is not necessary to be a comprehensive one. Don't hesitate to submit.

I looked over the patch and was wondering why you chose to have a
configure option to disable row-level ACLs. I assume that could just be
always enabled. In fact, perhaps that is the first part of the patch
that should be applied because it doesn't rely on SE-Linux.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> KaiGai Kohei wrote:
>> I updated the patch set of SE-PostgreSQL (revision 1268).
>>
>> [1/6] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1268.patch
>> [2/6] http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r1268.patch
>> [3/6] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1268.patch
>> [4/6] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1268.patch
>> [5/6] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1268.patch
>> [6/6] http://sepgsql.googlecode.com/files/sepostgresql-row_acl-8.4devel-3-r1268.patch
>>
>> Draft of the SE-PostgreSQL documentation is here:
>> http://wiki.postgresql.org/wiki/SEPostgreSQL
>>
>> List of updates:
>> - The patches are rebased to the CVS HEAD.
>> - RelOptions related hooks are cleaned up.
>> - The Row-level ACL feature is chosen in default.
>> - rowacl_table_default() is added to show the default ACL of the table.
>> - The initial revision of regression test for Row-level ACL is added.
>>
>> If you have anything to comment for the patches, could you disclose it?
>> It is not necessary to be a comprehensive one. Don't hesitate to submit.
>
> I looked over the patch and was wondering why you chose to have a
> configure option to disable row-level ACLs.

There are no explicit reasons.
I thought it was natural, as if we can build Linux kernel without any
enhanced security features (SELinux, SMACK and so on).

I don't oppose to elimination of "--disable-row-acl" options, however,
it is not clear for me whether it should be unavoidable selection in
the future, or not.

> I assume that could just be always enabled.

It is not "always" enabled. When we build it with SE-PostgreSQL feature,
rest of enhanced security features (includes the row-level ACL) are
disabled automatically, as we discussed before.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> >> If you have anything to comment for the patches, could you disclose it?
> >> It is not necessary to be a comprehensive one. Don't hesitate to submit.
> >
> > I looked over the patch and was wondering why you chose to have a
> > configure option to disable row-level ACLs.
>
> There are no explicit reasons.
> I thought it was natural, as if we can build Linux kernel without any
> enhanced security features (SELinux, SMACK and so on).
>
> I don't oppose to elimination of "--disable-row-acl" options, however,
> it is not clear for me whether it should be unavoidable selection in
> the future, or not.

Look at the existing configure options; we don't remove features via
configure unless it is for some platform-specific reason. Please remove
the configure option and make it always enabled. The way it should work
is that the feature is always enabled, and some SQL-level commands
should throw an appropriate error if SE-Linux is enabled in the build.

> > I assume that could just be always enabled.
>
> It is not "always" enabled. When we build it with SE-PostgreSQL feature,
> rest of enhanced security features (includes the row-level ACL) are
> disabled automatically, as we discussed before.

Oh. Is that because we use SE-Linux row-level security when
SE-PostgreSQL is enabled? I guess that makes sense.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

>> I don't oppose to elimination of "--disable-row-acl" options, however,
>> it is not clear for me whether it should be unavoidable selection in
>> the future, or not.
>
> Look at the existing configure options; we don't remove features via
> configure unless it is for some platform-specific reason. Please remove
> the configure option and make it always enabled.

OK, I'll update it in the next patch set.

>>> I assume that could just be always enabled.
>> It is not "always" enabled. When we build it with SE-PostgreSQL feature,
>> rest of enhanced security features (includes the row-level ACL) are
>> disabled automatically, as we discussed before.
>
> Oh. Is that because we use SE-Linux row-level security when
> SE-PostgreSQL is enabled? I guess that makes sense.

Yes, when SE-PostgreSQL is enabled, it provides row-level security,
and the per-tuple security field is used to show its security context.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> >> I don't oppose to elimination of "--disable-row-acl" options, however,
> >> it is not clear for me whether it should be unavoidable selection in
> >> the future, or not.
> >
> > Look at the existing configure options; we don't remove features via
> > configure unless it is for some platform-specific reason. Please remove
> > the configure option and make it always enabled.
>
> OK, I'll update it in the next patch set.

Good. I assume the SQL-row security patch is not testable alone with
out the rest of the patches, right?

> >>> I assume that could just be always enabled.
> >> It is not "always" enabled. When we build it with SE-PostgreSQL feature,
> >> rest of enhanced security features (includes the row-level ACL) are
> >> disabled automatically, as we discussed before.
> >
> > Oh. Is that because we use SE-Linux row-level security when
> > SE-PostgreSQL is enabled? I guess that makes sense.
>
> Yes, when SE-PostgreSQL is enabled, it provides row-level security,
> and the per-tuple security field is used to show its security context.

Yes, that seems fine.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> KaiGai Kohei wrote:
>>>> I don't oppose to elimination of "--disable-row-acl" options, however,
>>>> it is not clear for me whether it should be unavoidable selection in
>>>> the future, or not.
>>> Look at the existing configure options; we don't remove features via
>>> configure unless it is for some platform-specific reason. Please remove
>>> the configure option and make it always enabled.
>> OK, I'll update it in the next patch set.
>
> Good. I assume the SQL-row security patch is not testable alone with
> out the rest of the patches, right?

The minimum requirements are the 1st and 2nd patches.
The first provides security hooks to PostgreSQL server program, and
the other provides ones to pg_dump command.
The 3rd, 4th and 5th are not necessary for the test purpose.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

I updated the patch set of SE-PostgreSQL and related (r1280)

[1/6] http://sepgsql.googlecode.com/files/sepostgresql-sepgsql-8.4devel-3-r1280.patch
[2/6] http://sepgsql.googlecode.com/files/sepostgresql-pg_dump-8.4devel-3-r1280.patch
[3/6] http://sepgsql.googlecode.com/files/sepostgresql-policy-8.4devel-3-r1280.patch
[4/6] http://sepgsql.googlecode.com/files/sepostgresql-docs-8.4devel-3-r1280.patch
[5/6] http://sepgsql.googlecode.com/files/sepostgresql-tests-8.4devel-3-r1280.patch
[6/6] http://sepgsql.googlecode.com/files/sepostgresql-row_acl-8.4devel-3-r1280.patch

Draft of the SE-PostgreSQL documentation is here:
http://wiki.postgresql.org/wiki/SEPostgreSQL

List of updates:
- The patches are rebased to the current CVS HEAD
- '--disable-row-acl' is removed. This feature is implicitly enabled
when we don't activate any other enhanced security features.
From this change, SECURITY_SYSATTR_NAME got being always defined,
so some of unnecessary #ifdef ... #endif block has gone.
- bugfix: it caused a crash on COPY when we specify a system attribute
on FORCE QUATE clause.
- cleanup: some warnings to notice unused variables are eliminated.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> Bruce Momjian wrote:
> > KaiGai Kohei wrote:
> >>>> I don't oppose to elimination of "--disable-row-acl" options, however,
> >>>> it is not clear for me whether it should be unavoidable selection in
> >>>> the future, or not.
> >>> Look at the existing configure options; we don't remove features via
> >>> configure unless it is for some platform-specific reason. Please remove
> >>> the configure option and make it always enabled.
> >> OK, I'll update it in the next patch set.
> >
> > Good. I assume the SQL-row security patch is not testable alone with
> > out the rest of the patches, right?
>
> The minimum requirements are the 1st and 2nd patches.
> The first provides security hooks to PostgreSQL server program, and
> the other provides ones to pg_dump command.
> The 3rd, 4th and 5th are not necessary for the test purpose.

First, let me say you have done an amazing job of producing patches for
us, and your code quality is very high, especially considering the
complexity of this code and your newness to our development process. My
compliments to NEC, your employer.

Also, I personally am excited about this code and what it will add to
Postgres 8.4.

I hate to ask for something else from you, but I am trying to figure out
how we can proceed in reviewing and applying your additions. I am
wondering if you can produce a patch that has the SE-Linux part separate
so I can review the non-SE-Linux parts of the patch alone --- right now
I am not 100% clear on what parts are always active as row-level SQL
security and what needs SE-Linux to operate. I know this is an
additional burden on you and if it is too much to ask, please tell me.

Thanks.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> KaiGai Kohei wrote:
>> Bruce Momjian wrote:
>>> KaiGai Kohei wrote:
>>>>>> I don't oppose to elimination of "--disable-row-acl" options, however,
>>>>>> it is not clear for me whether it should be unavoidable selection in
>>>>>> the future, or not.
>>>>> Look at the existing configure options; we don't remove features via
>>>>> configure unless it is for some platform-specific reason. Please remove
>>>>> the configure option and make it always enabled.
>>>> OK, I'll update it in the next patch set.
>>> Good. I assume the SQL-row security patch is not testable alone with
>>> out the rest of the patches, right?
>> The minimum requirements are the 1st and 2nd patches.
>> The first provides security hooks to PostgreSQL server program, and
>> the other provides ones to pg_dump command.
>> The 3rd, 4th and 5th are not necessary for the test purpose.
>
> First, let me say you have done an amazing job of producing patches for
> us, and your code quality is very high, especially considering the
> complexity of this code and your newness to our development process. My
> compliments to NEC, your employer.
>
> Also, I personally am excited about this code and what it will add to
> Postgres 8.4.
>
> I hate to ask for something else from you, but I am trying to figure out
> how we can proceed in reviewing and applying your additions. I am
> wondering if you can produce a patch that has the SE-Linux part separate
> so I can review the non-SE-Linux parts of the patch alone --- right now
> I am not 100% clear on what parts are always active as row-level SQL
> security and what needs SE-Linux to operate. I know this is an
> additional burden on you and if it is too much to ask, please tell me.

All the SELinux specific part is stored within:
- src/include/security/sepgsq.h
- src/backend/security/sepgsql/*
- Blocks enclosed by "#if defined(HAVE_SELINUX)"
in src/include/security/pgace.h

SELinux related codes are never invoked without pgaceXXXX() hooks,
so you can simply ignore the above files/parts when you are under
the reviewing to non-SELinux parts.
Rest of changes are commonly needed to manage security attribute
and to inject security hooks.

In all honesty, I hesitate to separate the patch again into two
parts to be integrated later. I would be happy, if you suggested
it a half year ago, because this feature was suggested as two
separated patches in CommitFest:May. :(

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> > I hate to ask for something else from you, but I am trying to figure out
> > how we can proceed in reviewing and applying your additions. I am
> > wondering if you can produce a patch that has the SE-Linux part separate
> > so I can review the non-SE-Linux parts of the patch alone --- right now
> > I am not 100% clear on what parts are always active as row-level SQL
> > security and what needs SE-Linux to operate. I know this is an
> > additional burden on you and if it is too much to ask, please tell me.
>
> All the SELinux specific part is stored within:
> - src/include/security/sepgsq.h
> - src/backend/security/sepgsql/*
> - Blocks enclosed by "#if defined(HAVE_SELINUX)"
> in src/include/security/pgace.h
>
> SELinux related codes are never invoked without pgaceXXXX() hooks,
> so you can simply ignore the above files/parts when you are under
> the reviewing to non-SELinux parts.
> Rest of changes are commonly needed to manage security attribute
> and to inject security hooks.
>
> In all honesty, I hesitate to separate the patch again into two
> parts to be integrated later. I would be happy, if you suggested
> it a half year ago, because this feature was suggested as two
> separated patches in CommitFest:May. :(

Thanks, that's what I needed to know.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> Bruce Momjian wrote:
>> I assume that could just be always enabled.

> It is not "always" enabled. When we build it with SE-PostgreSQL feature,
> rest of enhanced security features (includes the row-level ACL) are
> disabled automatically, as we discussed before.

It seems like a pretty awful idea to have enabling sepostgres take away
a feature that exists in the default build.

regards, tom lane

Tom Lane wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>> Bruce Momjian wrote:
>>> I assume that could just be always enabled.
>
>> It is not "always" enabled. When we build it with SE-PostgreSQL feature,
>> rest of enhanced security features (includes the row-level ACL) are
>> disabled automatically, as we discussed before.
>
> It seems like a pretty awful idea to have enabling sepostgres take away
> a feature that exists in the default build.

Why?

The PGACE security framework allows one or no enhanced security
mechanism at most. It is quite natural that the default selection
is overrided when an alternative option is chosen explicitly.

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

On Tue, 2008-12-09 at 03:33 +0900, KaiGai Kohei wrote:
> Tom Lane wrote:
> > KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> >> Bruce Momjian wrote:
> >>> I assume that could just be always enabled.
> >
> >> It is not "always" enabled. When we build it with SE-PostgreSQL feature,
> >> rest of enhanced security features (includes the row-level ACL) are
> >> disabled automatically, as we discussed before.
> >
> > It seems like a pretty awful idea to have enabling sepostgres take away
> > a feature that exists in the default build.
>
> Why?
>
> The PGACE security framework allows one or no enhanced security
> mechanism at most. It is quite natural that the default selection
> is overrided when an alternative option is chosen explicitly.

I'm finding these discussions very confusing to follow, sorry about
that.

We now have a parameter option that allows you to have row level
security in non-mandatory mode, which is good. But in order to get that
we need to build the server with a special configure option.

My previous objective was to remove the need for a configure option, so
we can enable row-level security in the default distribution of
Postgres. Are we going to enable that option in all normal distros? If
yes, why is it a configure option (at all)?

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Tue, 2008-12-09 at 03:33 +0900, KaiGai Kohei wrote:
>> Tom Lane wrote:
>>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>>> Bruce Momjian wrote:
>>>>> I assume that could just be always enabled.
>>>> It is not "always" enabled. When we build it with SE-PostgreSQL feature,
>>>> rest of enhanced security features (includes the row-level ACL) are
>>>> disabled automatically, as we discussed before.
>>> It seems like a pretty awful idea to have enabling sepostgres take away
>>> a feature that exists in the default build.
>> Why?
>>
>> The PGACE security framework allows one or no enhanced security
>> mechanism at most. It is quite natural that the default selection
>> is overrided when an alternative option is chosen explicitly.
>
> I'm finding these discussions very confusing to follow, sorry about
> that.
>
> We now have a parameter option that allows you to have row level
> security in non-mandatory mode, which is good. But in order to get that
> we need to build the server with a special configure option.

We need to distinguish a selection of enhanced security mechanism
and options provided by the mechanism chosen.

The PGACE security framework allows one or no enhanced security
mechanism at most when it is built. Thus, we have to determine
what mechanism to be activated. Currently, we have two enhanced
security mechanism for v8.4. The one is SE-PostgreSQL, and the
other is Row-level ACLs.

SE-PostgreSQL is a MAC-based feature which provides column/row
level granularity and collaboration with operating system.
It provides two GUC parameter options as follows:
- sepostgresql = (default|enforcing|permissive|disabled)
- sepostgresql_row_level = (on|off)

Row-level ACLs is a DAC-based feature which provides row level
granularity and works independently from operating system security.
It provides two table options as follows:
- row_level_acl = (on|off)
- default_row_acl = '<ACL text>'

> My previous objective was to remove the need for a configure option, so
> we can enable row-level security in the default distribution of
> Postgres. Are we going to enable that option in all normal distros? If
> yes, why is it a configure option (at all)?

The purpose of configure options is to choose a enhanced security
mechanism implemented on the PGACE security framework.
It is unclear for me what means the "row-level security" in this
context. Is it provided by SE-PostgreSQL? Row-level ACLs?

We currently have no option to disable all the enhanced security
mechanism, so one mechanism has to be choosen at least.
Both of the two mechanisms being available now provide row-level
granularity, so "row-level security" will be always enabled *in the
result*. Please note that it can be provided by different security
mechanism which is designed with independent security model.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Simon Riggs wrote:
>
> On Tue, 2008-12-09 at 03:33 +0900, KaiGai Kohei wrote:
> > Tom Lane wrote:
> > > KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> > >> Bruce Momjian wrote:
> > >>> I assume that could just be always enabled.
> > >
> > >> It is not "always" enabled. When we build it with SE-PostgreSQL feature,
> > >> rest of enhanced security features (includes the row-level ACL) are
> > >> disabled automatically, as we discussed before.
> > >
> > > It seems like a pretty awful idea to have enabling sepostgres take away
> > > a feature that exists in the default build.
> >
> > Why?
> >
> > The PGACE security framework allows one or no enhanced security
> > mechanism at most. It is quite natural that the default selection
> > is overrided when an alternative option is chosen explicitly.
>
> I'm finding these discussions very confusing to follow, sorry about
> that.

It isn't you; it _is_ hard to follow. ;-) No one is at fault; the
topic is just complex.

> We now have a parameter option that allows you to have row level
> security in non-mandatory mode, which is good. But in order to get that
> we need to build the server with a special configure option.

No, that was removed at my request so it is always available.

> My previous objective was to remove the need for a configure option, so
> we can enable row-level security in the default distribution of
> Postgres. Are we going to enable that option in all normal distros? If
> yes, why is it a configure option (at all)?

Option is gone in the most recent patch and it is always enabled. We
still have a configure option to enable SE-Linux features.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> > Bruce Momjian wrote:
> >> I assume that could just be always enabled.
>
> > It is not "always" enabled. When we build it with SE-PostgreSQL feature,
> > rest of enhanced security features (includes the row-level ACL) are
> > disabled automatically, as we discussed before.
>
> It seems like a pretty awful idea to have enabling sepostgres take away
> a feature that exists in the default build.

Agreed. The problem is that the security column used for SQL-level row
security is reused to hold the SE-Linux ACL when SE-Linux is enabled. I
suppose the only way to enable them both in an SE-Linux build would be
to use a new optional column for SE-Linux and keep the SQL-level row
security optional column unchanged.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Tom Lane wrote:
>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>> Bruce Momjian wrote:
>>>> I assume that could just be always enabled.
>>> It is not "always" enabled. When we build it with SE-PostgreSQL feature,
>>> rest of enhanced security features (includes the row-level ACL) are
>>> disabled automatically, as we discussed before.
>> It seems like a pretty awful idea to have enabling sepostgres take away
>> a feature that exists in the default build.
>
> Agreed.

I don't agree. What is the reason why? It has been unclear for me.

The PGACE security framework is designed to allow users to choose
an enhanced security mechanism from some of provided options.
(Currently, we have sepgsql and rowacl.)
It is quite natural that one is disabled when the other is enabled.

If a specific enhanced security mechanism has a privileged position,
it should not be a guest of the security framwork, and be hardcoded
like existing table-level database ACLs.

Again, I don't oppose the Row-level ACLs to be the default selection.
However, it should be a selectable option.

Thanks,

> The problem is that the security column used for SQL-level row
> security is reused to hold the SE-Linux ACL when SE-Linux is enabled. I
> suppose the only way to enable them both in an SE-Linux build would be
> to use a new optional column for SE-Linux and keep the SQL-level row
> security optional column unchanged.
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Is there any reason it's easier to do as a configure option instead of
an initdb option or better yet a per-database option?

The reason we're shying away from configure options for functionality
changes is that more and more users are getting pistgres from binary
distributions. Which option should redhat ship for example?

Also, a configure option is kind if weird since that's a property of
the binary not the data: what happens if you build a database with
selinux and the switch binaries to a non-we build? Is that option in
pg_control?

--
Greg

On 10 Dec 2008, at 12:13, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:

> Bruce Momjian wrote:
>> Tom Lane wrote:
>>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>>> Bruce Momjian wrote:
>>>>> I assume that could just be always enabled.
>>>> It is not "always" enabled. When we build it with SE-PostgreSQL
>>>> feature,
>>>> rest of enhanced security features (includes the row-level ACL) are
>>>> disabled automatically, as we discussed before.
>>> It seems like a pretty awful idea to have enabling sepostgres take
>>> away
>>> a feature that exists in the default build.
>> Agreed.
>
> I don't agree. What is the reason why? It has been unclear for me.
>
> The PGACE security framework is designed to allow users to choose
> an enhanced security mechanism from some of provided options.
> (Currently, we have sepgsql and rowacl.)
> It is quite natural that one is disabled when the other is enabled.
>
> If a specific enhanced security mechanism has a privileged position,
> it should not be a guest of the security framwork, and be hardcoded
> like existing table-level database ACLs.
>
> Again, I don't oppose the Row-level ACLs to be the default selection.
> However, it should be a selectable option.
>
> Thanks,
>
>> The problem is that the security column used for SQL-level row
>> security is reused to hold the SE-Linux ACL when SE-Linux is
>> enabled. I
>> suppose the only way to enable them both in an SE-Linux build would
>> be
>> to use a new optional column for SE-Linux and keep the SQL-level row
>> security optional column unchanged.
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

KaiGai Kohei wrote:
> Bruce Momjian wrote:
> > Tom Lane wrote:
> >> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> >>> Bruce Momjian wrote:
> >>>> I assume that could just be always enabled.
> >>> It is not "always" enabled. When we build it with SE-PostgreSQL feature,
> >>> rest of enhanced security features (includes the row-level ACL) are
> >>> disabled automatically, as we discussed before.
> >> It seems like a pretty awful idea to have enabling sepostgres take away
> >> a feature that exists in the default build.
> >
> > Agreed.
>
> I don't agree. What is the reason why? It has been unclear for me.
>
> The PGACE security framework is designed to allow users to choose
> an enhanced security mechanism from some of provided options.
> (Currently, we have sepgsql and rowacl.)
> It is quite natural that one is disabled when the other is enabled.
>
> If a specific enhanced security mechanism has a privileged position,
> it should not be a guest of the security framwork, and be hardcoded
> like existing table-level database ACLs.
>
> Again, I don't oppose the Row-level ACLs to be the default selection.
> However, it should be a selectable option.

I understand, but imagine how this is going to interact for users. What
happens if I install an SE-Linux binary and point it at a /data
directory that was not created by SE-Linxu binary. How is the SE-Linux
binary going to interpret the security field? What happens if I load a
non-SE-Linux data dump into a SE-Linux binary? Do I lose my security
settings?

I am starting to think we should have two optional security fields, one
for SQL and one for SE-Linux. The big downside of that is that we are
back to the case of the having lots of SE-Linux-specific code to handle
that SE-Linux field, rather than reusing the SQL-row-level security
field.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Greg Stark wrote:
> Is there any reason it's easier to do as a configure option instead of
> an initdb option or better yet a per-database option?

SE-PostgreSQL needs "libselinux" to communicate with SELinux,
however, it is not available for non-SELinux'ed platforms.

> The reason we're shying away from configure options for functionality
> changes is that more and more users are getting pistgres from binary
> distributions. Which option should redhat ship for example?

As we're doing on Fedora, SE-PostgreSQL will be distributed as
a separated package.
If a user want to start SE- version, he will install another
binary package, and stop vanilla PostgreSQl, and start SE-PostgreSQL.

> Also, a configure option is kind if weird since that's a property of the
> binary not the data: what happens if you build a database with selinux
> and the switch binaries to a non-we build? Is that option in pg_control?

In this case, the vanilla PostgreSQL should be simply ignore the
data which is generated by SE-PostgreSQL.

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

KaiGai Kohei wrote:
> I don't agree. What is the reason why? It has been unclear for me.
>
> The PGACE security framework is designed to allow users to choose
> an enhanced security mechanism from some of provided options.
> (Currently, we have sepgsql and rowacl.)
> It is quite natural that one is disabled when the other is enabled.

As a general rule, mutually exclusive features as compile-time option
should be avoided at all costs. Since most people use binary packages,
forcing the packager to make such a choice will always make a lot of
people unhappy, or alternatively cause one of the features to bitrot.

As a secondary rule, mutually exclusive features should be avoided at
all, without a compelling reason. I don't see such a reason here.

Peter Eisentraut wrote:
> KaiGai Kohei wrote:
> > I don't agree. What is the reason why? It has been unclear for me.
> >
> > The PGACE security framework is designed to allow users to choose
> > an enhanced security mechanism from some of provided options.
> > (Currently, we have sepgsql and rowacl.)
> > It is quite natural that one is disabled when the other is enabled.
>
> As a general rule, mutually exclusive features as compile-time option
> should be avoided at all costs. Since most people use binary packages,
> forcing the packager to make such a choice will always make a lot of
> people unhappy, or alternatively cause one of the features to bitrot.
>
> As a secondary rule, mutually exclusive features should be avoided at
> all, without a compelling reason. I don't see such a reason here.

I think there is a reason to have SE-Linux be compile-time because there
is no way to know at run time if the OS has the SE-Linux libraries,
right? I assume this is similar to how we do LDAP.

But your larger point is that SQL-row-level security should always be
available, which I just posted about.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> KaiGai Kohei wrote:
>> Bruce Momjian wrote:
>>> Tom Lane wrote:
>>>> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>>>>> Bruce Momjian wrote:
>>>>>> I assume that could just be always enabled.
>>>>> It is not "always" enabled. When we build it with SE-PostgreSQL feature,
>>>>> rest of enhanced security features (includes the row-level ACL) are
>>>>> disabled automatically, as we discussed before.
>>>> It seems like a pretty awful idea to have enabling sepostgres take away
>>>> a feature that exists in the default build.
>>> Agreed.
>> I don't agree. What is the reason why? It has been unclear for me.
>>
>> The PGACE security framework is designed to allow users to choose
>> an enhanced security mechanism from some of provided options.
>> (Currently, we have sepgsql and rowacl.)
>> It is quite natural that one is disabled when the other is enabled.
>>
>> If a specific enhanced security mechanism has a privileged position,
>> it should not be a guest of the security framwork, and be hardcoded
>> like existing table-level database ACLs.
>>
>> Again, I don't oppose the Row-level ACLs to be the default selection.
>> However, it should be a selectable option.
>
> I understand, but imagine how this is going to interact for users. What
> happens if I install an SE-Linux binary and point it at a /data
> directory that was not created by SE-Linxu binary. How is the SE-Linux
> binary going to interpret the security field?

When SE-PostgreSQL binary fetch a tuple without its security attribute,
it considers the tuple has an alternative one called as "unlabeled_t".
This behavior is same as when we mount an unlabled filesystem on SELinux
system.

> What happens if I load a non-SE-Linux data dump into a SE-Linux binary?
> Do I lose my security settings?

It is same as normal INSERT case. When user gives a data without specific
security context, SE-PostgreSQL assigns it a default security context.
In the default security policy, it is "sepgsql_table_t".

> I am starting to think we should have two optional security fields, one
> for SQL and one for SE-Linux. The big downside of that is that we are
> back to the case of the having lots of SE-Linux-specific code to handle
> that SE-Linux field, rather than reusing the SQL-row-level security
> field.

It is just an idea. If Row-level ACL feature is *hardcoded* (not a guest
of PGACE), is it considerable a hidden attribute typed as "aclitem[]"?

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

Bruce Momjian wrote:
> Peter Eisentraut wrote:
>> KaiGai Kohei wrote:
>>> I don't agree. What is the reason why? It has been unclear for me.
>>>
>>> The PGACE security framework is designed to allow users to choose
>>> an enhanced security mechanism from some of provided options.
>>> (Currently, we have sepgsql and rowacl.)
>>> It is quite natural that one is disabled when the other is enabled.
>> As a general rule, mutually exclusive features as compile-time option
>> should be avoided at all costs. Since most people use binary packages,
>> forcing the packager to make such a choice will always make a lot of
>> people unhappy, or alternatively cause one of the features to bitrot.
>>
>> As a secondary rule, mutually exclusive features should be avoided at
>> all, without a compelling reason. I don't see such a reason here.
>
> I think there is a reason to have SE-Linux be compile-time because there
> is no way to know at run time if the OS has the SE-Linux libraries,
> right? I assume this is similar to how we do LDAP.

Yes, the libselinux is a factor it to be a compile-time option.

> But your larger point is that SQL-row-level security should always be
> available, which I just posted about.

If so, it should be hardcoded on somewhere, no need to be implemented
as a guest of PGACE security framework. Its purpose is to implement
enhanced security mechanisms with minimum impact to core facilities.

If you intend to implement is as a hardcoded feature, I can agree.
Please wait for a few days, I'll try to implement it.
So, ignore the 6th patch during the days and make progress to review
the rest of patches.

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

KaiGai Kohei wrote:
> > But your larger point is that SQL-row-level security should always be
> > available, which I just posted about.
>
> If so, it should be hardcoded on somewhere, no need to be implemented
> as a guest of PGACE security framework. Its purpose is to implement
> enhanced security mechanisms with minimum impact to core facilities.
>
> If you intend to implement is as a hardcoded feature, I can agree.
> Please wait for a few days, I'll try to implement it.
> So, ignore the 6th patch during the days and make progress to review
> the rest of patches.

Hold, please don't code yet. You might have already read our
developer's FAQ:

http://wiki.postgresql.org/wiki/Developer_FAQ#What_do_I_do_after_choosing_an_item_to_work_on.3F

but I feel we need to follow part of it to prevent you from repeatedly
having to adjust your patch, which I think has happened in the past.

Let's decide exactly what configure options, GUC options, system catalog
changes, and user-visible SQL command syntax we are going to use for the
patch before you recode anything.

For example, what configure flags are we going to have? I assume just
'--enable-selinux', right?

Second, system catalogs; are you going to have separate columns for
SQL-level row security and SE-Linux security? If so, is the SE-Linux
column only going to be created by the --enable-selinux build? If so,
that is going to mean that the /data directory is going to be affected
by the --enable-selinux flag, which is not ideal --- it would be good if
someone could switch to an SE-Linux binary without to dump/reload. One
nice idea would be to have one "security" column and allow both
SQL-level and SE-Linux security values to be stored in the column,
perhaps at the same time; that way there is no new column needed for
SE-Linux.

I know I suggested the security column act like the system oid column,
but now I am thinking I was wrong. The system oid column stores an
auto-generated value so it was necessary to have it be specified at
CREATE TABLE time because it is guaranteed to take storage space. For
the security column, in most cases it will be null, meaning it would not
take any storage space becuase we use a null bitmap for null column
values. So, perhaps the security column should be in every table and we
can just make it default to null; I think that would give us much more
flexibility to add security to existing tables.

Finally, I am wondering what other things should be adjusted that I have
not thought of yet. I would like to make KaiGai's job as easy as
possible.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Second, system catalogs; are you going to have separate columns for
> SQL-level row security and SE-Linux security? If so, is the SE-Linux
> column only going to be created by the --enable-selinux build? If so,
> that is going to mean that the /data directory is going to be affected
> by the --enable-selinux flag, which is not ideal --- it would be good if
> someone could switch to an SE-Linux binary without to dump/reload. One
> nice idea would be to have one "security" column and allow both
> SQL-level and SE-Linux security values to be stored in the column,
> perhaps at the same time; that way there is no new column needed for
> SE-Linux.
>
> I know I suggested the security column act like the system oid column,
> but now I am thinking I was wrong. The system oid column stores an
> auto-generated value so it was necessary to have it be specified at
> CREATE TABLE time because it is guaranteed to take storage space. For
> the security column, in most cases it will be null, meaning it would not
> take any storage space becuase we use a null bitmap for null column
> values. So, perhaps the security column should be in every table and we
> can just make it default to null; I think that would give us much more
> flexibility to add security to existing tables.

Sorry, I am confusing system columns, like pg_class.relname, with
user-table system columns, like mytable.xmin. I don't think we have the
ability of having user-table system columns nullable, right?

We do have a per-row HEAP_HASOID bit, so I wonder if we can have a
HEAP_HASSEC bit too. Right now the HEAP_HASOID is controlled by the
CREATE/ALTER table; I am unclear how hard it would be to allow it to be
controlled via INSERT, meaning it would be present only if the row had a
SEC value supplied.

Here is our OID column on some rows but not others:

test=> CREATE TABLE test (x INT) WITH oids;
CREATE TABLE
test=> INSERT INTO test VALUES (1);
INSERT 16392 1
test=> ALTER TABLE test SET WITHOUT OIDS;
ALTER TABLE
test=> INSERT INTO test VALUES(2);
INSERT 0 1
test=> SELECT oid, * FROM test;
ERROR: COLUMN "oid" does not exist
LINE 1: SELECT oid, * FROM test;

but it has per-table granularity; the oid value for '1' is still
stored, but is no longer visible; that would not work for the security
value.

The complexity is that the column would always exist (unlike OID), but
would be NULL if not assigned to. This would allow any table to have
security by default, and have no overhead if security is not defined for
the row. Now, I realize SE-Linux would have security defined for each
row, but SQL-level row security might not, and this would allow the
feature to be very useful, perhaps with a GUC that required security for
SE-Linux, and /data would be the same for SE-Linux and non-SE-Linux. If
we could make the row behave like this we might use separate columns for
SQL-level and SE-Linux security because there would be no storage
overhead unless security was used.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> KaiGai Kohei wrote:
>>> But your larger point is that SQL-row-level security should always be
>>> available, which I just posted about.
>> If so, it should be hardcoded on somewhere, no need to be implemented
>> as a guest of PGACE security framework. Its purpose is to implement
>> enhanced security mechanisms with minimum impact to core facilities.
>>
>> If you intend to implement is as a hardcoded feature, I can agree.
>> Please wait for a few days, I'll try to implement it.
>> So, ignore the 6th patch during the days and make progress to review
>> the rest of patches.
>
> Hold, please don't code yet. You might have already read our
> developer's FAQ:
>
> http://wiki.postgresql.org/wiki/Developer_FAQ#What_do_I_do_after_choosing_an_item_to_work_on.3F
>
> but I feel we need to follow part of it to prevent you from repeatedly
> having to adjust your patch, which I think has happened in the past.
>
> Let's decide exactly what configure options, GUC options, system catalog
> changes, and user-visible SQL command syntax we are going to use for the
> patch before you recode anything.

The guest of PGACE security framework should be chosen by configure option.
It also means any other facilities except for the guest of PGACE can be
enabled/disabled by other kind of options.

If the Row-level ACLs should be always enabled (independent from SE-PostgreSQL),
I think it should be hardcoded outside of PGACE, and enabled/disabled by
any other way. Table option is a candidate, like:

CREATE TABLE t (
a int,
b text
) WITH (ROW_LEVEL_ACL=ON);

> For example, what configure flags are we going to have? I assume just
> '--enable-selinux', right?

Yes, currently.

> Second, system catalogs; are you going to have separate columns for
> SQL-level row security and SE-Linux security?

Yes, I think these different properties are stored within separate
columns, as if a file has both of permission masks (-rwxr-x---) and
security context of SELinux.

> If so, is the SE-Linux
> column only going to be created by the --enable-selinux build? If so,
> that is going to mean that the /data directory is going to be affected
> by the --enable-selinux flag, which is not ideal --- it would be good if
> someone could switch to an SE-Linux binary without to dump/reload. One
> nice idea would be to have one "security" column and allow both
> SQL-level and SE-Linux security values to be stored in the column,
> perhaps at the same time; that way there is no new column needed for
> SE-Linux.

As you noticed, a "conditional system column" can be open to discuss.
If someone switch his binary to SE- version, the tables already defined
in $PGDATA don't have "security_context" system column. SE-PostgreSQL
handles tuples stored within the table as "unlabeled" one, but it does
not allow users to access the attribute due to lack of proper system
column.

In addition, we may have to refer security context of tuples via
"tuple_acl" system column, when someone switch his binary from
the Row-level ACL to SE-PostgreSQL. :)

The naming is a difficult matter. It seems to me "security" is too
general term. How do you think "security_label" or "security_attribute"?

> I know I suggested the security column act like the system oid column,
> but now I am thinking I was wrong. The system oid column stores an
> auto-generated value so it was necessary to have it be specified at
> CREATE TABLE time because it is guaranteed to take storage space. For
> the security column, in most cases it will be null, meaning it would not
> take any storage space becuase we use a null bitmap for null column
> values. So, perhaps the security column should be in every table and we
> can just make it default to null; I think that would give us much more
> flexibility to add security to existing tables.

Here is a differences in frequency of valid value in security column
between SE-PostgreSQL and Row-level ACLs.
SE-PostgreSQL requires all the tuple should be labeled properly.
(And, I guess any other upcoming feature to support another secure OS
has similar requirement.)
But we assume the Row-level ACLs is enabled on the limited number of
tables, so it need to specify an option to activate on CREATE TABLE.

Please note that I distinguish between "security column" and "acl column"
here. In my opinion, the security column should be a system column, but
the acl column can be implemented as a regular column that is appended
depending on user's needs.

When we represent a NULL of "security column", it requires HEAP_HASSECURITY
to be set on t_infomask. Here is no additional storage consumption.
If it is represented as NULL bitmaps, it requires a tuple has its null bitmaps
which need 4-bytes at least, so it can make additional storage consumption.
Thus, NULL-bitmap approach is not suitable to represent "security column"
because it has to be defined for any tables. But, it can be suitable for
"acl column".

The following proposal is my idea:

- The Row-level ACLs is implemented as a hardcoded feature, not a guest of
PGACE security framework.
- It can be enabled/disabled via table options as:
CREATE TABLE t (
a int,
b text
) WITH (ROW_LEVEL_ACL=ON);
- When the Row-level ACLs is enabled on CREATE TABLE, it implicitly add
a column to represent Row-level ACLs. This column is defined as aclitem[].
- The "acl column" is not expanded via "SELECT * FROM ...", but we can specify
it explicitly like "SELECT tuple_acl, * FROM ..." as system column doing.
- The "acl column" is allowed to update by the table owner or superuser.
- If a table has Row-level ACLs enabled, ExecScan() checks visibility of
fetched tuples, and ignore violated tuples.

> Finally, I am wondering what other things should be adjusted that I have
> not thought of yet. I would like to make KaiGai's job as easy as
> possible.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Bruce Momjian wrote:
> Bruce Momjian wrote:
>> Second, system catalogs; are you going to have separate columns for
>> SQL-level row security and SE-Linux security? If so, is the SE-Linux
>> column only going to be created by the --enable-selinux build? If so,
>> that is going to mean that the /data directory is going to be affected
>> by the --enable-selinux flag, which is not ideal --- it would be good if
>> someone could switch to an SE-Linux binary without to dump/reload. One
>> nice idea would be to have one "security" column and allow both
>> SQL-level and SE-Linux security values to be stored in the column,
>> perhaps at the same time; that way there is no new column needed for
>> SE-Linux.
>>
>> I know I suggested the security column act like the system oid column,
>> but now I am thinking I was wrong. The system oid column stores an
>> auto-generated value so it was necessary to have it be specified at
>> CREATE TABLE time because it is guaranteed to take storage space. For
>> the security column, in most cases it will be null, meaning it would not
>> take any storage space becuase we use a null bitmap for null column
>> values. So, perhaps the security column should be in every table and we
>> can just make it default to null; I think that would give us much more
>> flexibility to add security to existing tables.
>
> Sorry, I am confusing system columns, like pg_class.relname, with
> user-table system columns, like mytable.xmin. I don't think we have the
> ability of having user-table system columns nullable, right?

Currently, it assumes system columns should not be NULL.
heap_getsysattr() always set the given 'isnull' 'false'.

> We do have a per-row HEAP_HASOID bit, so I wonder if we can have a
> HEAP_HASSEC bit too. Right now the HEAP_HASOID is controlled by the
> CREATE/ALTER table;

The current patch add HEAP_HASSECURITY bit to t_infomask. :-)
When it is false, its security field is not available and not allocated.

> I am unclear how hard it would be to allow it to be
> controlled via INSERT, meaning it would be present only if the row had a
> SEC value supplied.

It is impossible, and not suitable for SE-PostgreSQL.
The HeapTuple is allocated prior to fetch INSERT'ed value.
In addition, SE-PostgreSQL assigns its default security context when no
security attribute is given.

> Here is our OID column on some rows but not others:
>
> test=> CREATE TABLE test (x INT) WITH oids;
> CREATE TABLE
> test=> INSERT INTO test VALUES (1);
> INSERT 16392 1
> test=> ALTER TABLE test SET WITHOUT OIDS;
> ALTER TABLE
> test=> INSERT INTO test VALUES(2);
> INSERT 0 1
> test=> SELECT oid, * FROM test;
> ERROR: COLUMN "oid" does not exist
> LINE 1: SELECT oid, * FROM test;
>
> but it has per-table granularity; the oid value for '1' is still
> stored, but is no longer visible; that would not work for the security
> value.

If we can control the Row-level ACLs via table options, what should be
happen when we turn off the feature?
IMO, it is natural to ignore Row-level ACLs independent from whether
the stored tuple actually has ACLs, or not.

> The complexity is that the column would always exist (unlike OID), but
> would be NULL if not assigned to. This would allow any table to have
> security by default, and have no overhead if security is not defined for
> the row. Now, I realize SE-Linux would have security defined for each
> row, but SQL-level row security might not, and this would allow the
> feature to be very useful, perhaps with a GUC that required security for
> SE-Linux, and /data would be the same for SE-Linux and non-SE-Linux. If
> we could make the row behave like this we might use separate columns for
> SQL-level and SE-Linux security because there would be no storage
> overhead unless security was used.

I basically agree for the idea of separate columns.
My preference is "acl column" as general column (not a system column)
which is defined at limited number of tables with ROW_LEVEL_ACL=ON.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> > Let's decide exactly what configure options, GUC options, system catalog
> > changes, and user-visible SQL command syntax we are going to use for the
> > patch before you recode anything.
>
> The guest of PGACE security framework should be chosen by configure option.
> It also means any other facilities except for the guest of PGACE can be
> enabled/disabled by other kind of options.
>
> If the Row-level ACLs should be always enabled (independent from SE-PostgreSQL),
> I think it should be hardcoded outside of PGACE, and enabled/disabled by
> any other way. Table option is a candidate, like:
>
> CREATE TABLE t (
> a int,
> b text
> ) WITH (ROW_LEVEL_ACL=ON);

As I mentioned below, this might not be necessary, meaning that row
security is enabled for rows where you set the row security value and
does not need to be turned on at create table time.

> > Second, system catalogs; are you going to have separate columns for
> > SQL-level row security and SE-Linux security?
>
> Yes, I think these different properties are stored within separate
> columns, as if a file has both of permission masks (-rwxr-x---) and
> security context of SELinux.

OK.

> > If so, is the SE-Linux
> > column only going to be created by the --enable-selinux build? If so,
> > that is going to mean that the /data directory is going to be affected
> > by the --enable-selinux flag, which is not ideal --- it would be good if
> > someone could switch to an SE-Linux binary without to dump/reload. One
> > nice idea would be to have one "security" column and allow both
> > SQL-level and SE-Linux security values to be stored in the column,
> > perhaps at the same time; that way there is no new column needed for
> > SE-Linux.
>
> As you noticed, a "conditional system column" can be open to discuss.
> If someone switch his binary to SE- version, the tables already defined
> in $PGDATA don't have "security_context" system column. SE-PostgreSQL
> handles tuples stored within the table as "unlabeled" one, but it does
> not allow users to access the attribute due to lack of proper system
> column.

Right, if the "security_context" always exists, but is null, moving to
SE-Linux would allow them to add security without dump/reload.

> In addition, we may have to refer security context of tuples via
> "tuple_acl" system column, when someone switch his binary from
> the Row-level ACL to SE-PostgreSQL. :)
>
> The naming is a difficult matter. It seems to me "security" is too
> general term. How do you think "security_label" or "security_attribute"?

So "security_context" has to be the PGACE column name. I would say
security_acl or just secacl because I think it is going to match our
existing ACL system pretty closely.

> > I know I suggested the security column act like the system oid column,
> > but now I am thinking I was wrong. The system oid column stores an
> > auto-generated value so it was necessary to have it be specified at
> > CREATE TABLE time because it is guaranteed to take storage space. For
> > the security column, in most cases it will be null, meaning it would not
> > take any storage space because we use a null bitmap for null column
> > values. So, perhaps the security column should be in every table and we
> > can just make it default to null; I think that would give us much more
> > flexibility to add security to existing tables.
>
> Here is a differences in frequency of valid value in security column
> between SE-PostgreSQL and Row-level ACLs.
> SE-PostgreSQL requires all the tuple should be labeled properly.
> (And, I guess any other upcoming feature to support another secure OS
> has similar requirement.)
> But we assume the Row-level ACLs is enabled on the limited number of
> tables, so it need to specify an option to activate on CREATE TABLE.
>
> Please note that I distinguish between "security column" and "acl column"
> here. In my opinion, the security column should be a system column, but
> the acl column can be implemented as a regular column that is appended
> depending on user's needs.

If we can make the column not take any space if it is null then we can
have both columns always present and that way /data is the same for
SE-Linux and non-SE-Linux. We do have columns like xmin which don't
show in SELECT * and I don't see a problem adding two more.

I imagine we would need to modify COPY and pg_dump to specify if we are
to dump/load secacl; I see you already did "security_context".

> When we represent a NULL of "security column", it requires HEAP_HASSECURITY
> to be set on t_infomask. Here is no additional storage consumption.

Nice!

> If it is represented as NULL bitmaps, it requires a tuple has its null bitmaps
> which need 4-bytes at least, so it can make additional storage consumption.
> Thus, NULL-bitmap approach is not suitable to represent "security column"
> because it has to be defined for any tables. But, it can be suitable for
> "acl column".

Ah, that is a good point, that if we have "security column" which is
usually null then we are requiring the NULL bitmask.

I was thinking of having them be system columns and therefore only the
bitmask would specify if the value exists or not. I am again thinking
of having both columns always exist, making your code clearer and more
portable for people going to and leaving SE-Postgres.

> The following proposal is my idea:
>
> - The Row-level ACLs is implemented as a hardcoded feature, not a guest of
> PGACE security framework.

Yep, good.

> - It can be enabled/disabled via table options as:
> CREATE TABLE t (
> a int,
> b text
> ) WITH (ROW_LEVEL_ACL=ON);

Can we make it just always on? See above.

> - When the Row-level ACLs is enabled on CREATE TABLE, it implicitly add
> a column to represent Row-level ACLs. This column is defined as aclitem[].
> - The "acl column" is not expanded via "SELECT * FROM ...", but we can specify
> it explicitly like "SELECT tuple_acl, * FROM ..." as system column doing.

Right, just like other system columns, e.g. xmin.

> - The "acl column" is allowed to update by the table owner or superuser.

Yep.

> - If a table has Row-level ACLs enabled, ExecScan() checks visibility of
> fetched tuples, and ignore violated tuples.

Yep.

> > Finally, I am wondering what other things should be adjusted that I have
> > not thought of yet. I would like to make KaiGai's job as easy as
> > possible.

I feel there must still be open issues that I hope we can address before
you need to do more recoding of the patch. And hopefully other
community members will be involved as well.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

KaiGai Kohei wrote:
> Bruce Momjian wrote:
> > Bruce Momjian wrote:
> >> Second, system catalogs; are you going to have separate columns for
> >> SQL-level row security and SE-Linux security? If so, is the SE-Linux
> >> column only going to be created by the --enable-selinux build? If so,
> >> that is going to mean that the /data directory is going to be affected
> >> by the --enable-selinux flag, which is not ideal --- it would be good if
> >> someone could switch to an SE-Linux binary without to dump/reload. One
> >> nice idea would be to have one "security" column and allow both
> >> SQL-level and SE-Linux security values to be stored in the column,
> >> perhaps at the same time; that way there is no new column needed for
> >> SE-Linux.
> >>
> >> I know I suggested the security column act like the system oid column,
> >> but now I am thinking I was wrong. The system oid column stores an
> >> auto-generated value so it was necessary to have it be specified at
> >> CREATE TABLE time because it is guaranteed to take storage space. For
> >> the security column, in most cases it will be null, meaning it would not
> >> take any storage space becuase we use a null bitmap for null column
> >> values. So, perhaps the security column should be in every table and we
> >> can just make it default to null; I think that would give us much more
> >> flexibility to add security to existing tables.
> >
> > Sorry, I am confusing system columns, like pg_class.relname, with
> > user-table system columns, like mytable.xmin. I don't think we have the
> > ability of having user-table system columns nullable, right?
>
> Currently, it assumes system columns should not be NULL.
> heap_getsysattr() always set the given 'isnull' 'false'.

Yep, that is a problem.

> > We do have a per-row HEAP_HASOID bit, so I wonder if we can have a
> > HEAP_HASSEC bit too. Right now the HEAP_HASOID is controlled by the
> > CREATE/ALTER table;
>
> The current patch add HEAP_HASSECURITY bit to t_infomask. :-)
> When it is false, its security field is not available and not allocated.

Good.

> > I am unclear how hard it would be to allow it to be
> > controlled via INSERT, meaning it would be present only if the row had a
> > SEC value supplied.
>
> It is impossible, and not suitable for SE-PostgreSQL.
> The HeapTuple is allocated prior to fetch INSERT'ed value.
> In addition, SE-PostgreSQL assigns its default security context when no
> security attribute is given.

When SE-Linux is enabled, couldn't NULL represent the default security
context?

> > Here is our OID column on some rows but not others:
> >
> > test=> CREATE TABLE test (x INT) WITH oids;
> > CREATE TABLE
> > test=> INSERT INTO test VALUES (1);
> > INSERT 16392 1
> > test=> ALTER TABLE test SET WITHOUT OIDS;
> > ALTER TABLE
> > test=> INSERT INTO test VALUES(2);
> > INSERT 0 1
> > test=> SELECT oid, * FROM test;
> > ERROR: COLUMN "oid" does not exist
> > LINE 1: SELECT oid, * FROM test;
> >
> > but it has per-table granularity; the oid value for '1' is still
> > stored, but is no longer visible; that would not work for the security
> > value.
>
> If we can control the Row-level ACLs via table options, what should be
> happen when we turn off the feature?
> IMO, it is natural to ignore Row-level ACLs independent from whether
> the stored tuple actually has ACLs, or not.

Hmmm, sounds like perhaps a GUC is needed there.

> > The complexity is that the column would always exist (unlike OID), but
> > would be NULL if not assigned to. This would allow any table to have
> > security by default, and have no overhead if security is not defined for
> > the row. Now, I realize SE-Linux would have security defined for each
> > row, but SQL-level row security might not, and this would allow the
> > feature to be very useful, perhaps with a GUC that required security for
> > SE-Linux, and /data would be the same for SE-Linux and non-SE-Linux. If
> > we could make the row behave like this we might use separate columns for
> > SQL-level and SE-Linux security because there would be no storage
> > overhead unless security was used.
>
> I basically agree for the idea of separate columns.
> My preference is "acl column" as general column (not a system column)
> which is defined at limited number of tables with ROW_LEVEL_ACL=ON.

That would certainly be the easiest approach.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> KaiGai Kohei wrote:
>>> Let's decide exactly what configure options, GUC options, system catalog
>>> changes, and user-visible SQL command syntax we are going to use for the
>>> patch before you recode anything.
>> The guest of PGACE security framework should be chosen by configure option.
>> It also means any other facilities except for the guest of PGACE can be
>> enabled/disabled by other kind of options.
>>
>> If the Row-level ACLs should be always enabled (independent from SE-PostgreSQL),
>> I think it should be hardcoded outside of PGACE, and enabled/disabled by
>> any other way. Table option is a candidate, like:
>>
>> CREATE TABLE t (
>> a int,
>> b text
>> ) WITH (ROW_LEVEL_ACL=ON);
>
> As I mentioned below, this might not be necessary, meaning that row
> security is enabled for rows where you set the row security value and
> does not need to be turned on at create table time.

The reason why I proposed the ROW_LEVEL_ACL option is to reduce storage
consumption for NULL bitmap. For example, the following INSERT originally
didn't need NULL bitmap, but the "acl column" is implicitly NULL, so the
HeapTuple has HEAP_HASNULL flag and NULL bitmap is allocated.

INSERT INTO t VALUES (1, 'aaa');

If it is acceptable, I don't think the option is necessary.

>>> If so, is the SE-Linux
>>> column only going to be created by the --enable-selinux build? If so,
>>> that is going to mean that the /data directory is going to be affected
>>> by the --enable-selinux flag, which is not ideal --- it would be good if
>>> someone could switch to an SE-Linux binary without to dump/reload. One
>>> nice idea would be to have one "security" column and allow both
>>> SQL-level and SE-Linux security values to be stored in the column,
>>> perhaps at the same time; that way there is no new column needed for
>>> SE-Linux.
>> As you noticed, a "conditional system column" can be open to discuss.
>> If someone switch his binary to SE- version, the tables already defined
>> in $PGDATA don't have "security_context" system column. SE-PostgreSQL
>> handles tuples stored within the table as "unlabeled" one, but it does
>> not allow users to access the attribute due to lack of proper system
>> column.
>
> Right, if the "security_context" always exists, but is null, moving to
> SE-Linux would allow them to add security without dump/reload.

Yes, but it is not recommendable in generally. For meaningful access
controls, "unlabeled" objects should be labeled properly. :)

>> In addition, we may have to refer security context of tuples via
>> "tuple_acl" system column, when someone switch his binary from
>> the Row-level ACL to SE-PostgreSQL. :)
>>
>> The naming is a difficult matter. It seems to me "security" is too
>> general term. How do you think "security_label" or "security_attribute"?
>
> So "security_context" has to be the PGACE column name. I would say
> security_acl or just secacl because I think it is going to match our
> existing ACL system pretty closely.

Agreed,

>>> I know I suggested the security column act like the system oid column,
>>> but now I am thinking I was wrong. The system oid column stores an
>>> auto-generated value so it was necessary to have it be specified at
>>> CREATE TABLE time because it is guaranteed to take storage space. For
>>> the security column, in most cases it will be null, meaning it would not
>>> take any storage space because we use a null bitmap for null column
>>> values. So, perhaps the security column should be in every table and we
>>> can just make it default to null; I think that would give us much more
>>> flexibility to add security to existing tables.
>> Here is a differences in frequency of valid value in security column
>> between SE-PostgreSQL and Row-level ACLs.
>> SE-PostgreSQL requires all the tuple should be labeled properly.
>> (And, I guess any other upcoming feature to support another secure OS
>> has similar requirement.)
>> But we assume the Row-level ACLs is enabled on the limited number of
>> tables, so it need to specify an option to activate on CREATE TABLE.
>>
>> Please note that I distinguish between "security column" and "acl column"
>> here. In my opinion, the security column should be a system column, but
>> the acl column can be implemented as a regular column that is appended
>> depending on user's needs.
>
> If we can make the column not take any space if it is null then we can
> have both columns always present and that way /data is the same for
> SE-Linux and non-SE-Linux. We do have columns like xmin which don't
> show in SELECT * and I don't see a problem adding two more.
>
> I imagine we would need to modify COPY and pg_dump to specify if we are
> to dump/load secacl; I see you already did "security_context".

Yes, it has to be also done.

>> If it is represented as NULL bitmaps, it requires a tuple has its null bitmaps
>> which need 4-bytes at least, so it can make additional storage consumption.
>> Thus, NULL-bitmap approach is not suitable to represent "security column"
>> because it has to be defined for any tables. But, it can be suitable for
>> "acl column".
>
> Ah, that is a good point, that if we have "security column" which is
> usually null then we are requiring the NULL bitmask.
>
> I was thinking of having them be system columns and therefore only the
> bitmask would specify if the value exists or not. I am again thinking
> of having both columns always exist, making your code clearer and more
> portable for people going to and leaving SE-Postgres.

The HEAP_HASSECURITY flag looks like a specific purpose NULL-bitmap
which only shows whether the tuple has security value, or not.

>> The following proposal is my idea:
>>
>> - The Row-level ACLs is implemented as a hardcoded feature, not a guest of
>> PGACE security framework.
>
> Yep, good.
>
>> - It can be enabled/disabled via table options as:
>> CREATE TABLE t (
>> a int,
>> b text
>> ) WITH (ROW_LEVEL_ACL=ON);
>
> Can we make it just always on? See above.

If we assume users set up Row-level ACLs for specific tables, per-table
option is meaningful for reduction of NULL-bitmap space in the tuple
without any NULL-values on general columns.

>> - When the Row-level ACLs is enabled on CREATE TABLE, it implicitly add
>> a column to represent Row-level ACLs. This column is defined as aclitem[].
>> - The "acl column" is not expanded via "SELECT * FROM ...", but we can specify
>> it explicitly like "SELECT tuple_acl, * FROM ..." as system column doing.
>
> Right, just like other system columns, e.g. xmin.
>
>> - The "acl column" is allowed to update by the table owner or superuser.
>
> Yep.
>
>> - If a table has Row-level ACLs enabled, ExecScan() checks visibility of
>> fetched tuples, and ignore violated tuples.
>
> Yep.
>
>>> Finally, I am wondering what other things should be adjusted that I have
>>> not thought of yet. I would like to make KaiGai's job as easy as
>>> possible.
>
> I feel there must still be open issues that I hope we can address before
> you need to do more recoding of the patch. And hopefully other
> community members will be involved as well.
>

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

>> > I am unclear how hard it would be to allow it to be
>> > controlled via INSERT, meaning it would be present only if the row had a
>> > SEC value supplied.
>>
>> It is impossible, and not suitable for SE-PostgreSQL.
>> The HeapTuple is allocated prior to fetch INSERT'ed value.
>> In addition, SE-PostgreSQL assigns its default security context when no
>> security attribute is given.
>
> When SE-Linux is enabled, couldn't NULL represent the default security
> context?

Yes, users will get the default security context, as if they insert
a tuple without any specific "oid" but proper value is assigned.

>>> Here is our OID column on some rows but not others:
>>>
>>> test=> CREATE TABLE test (x INT) WITH oids;
>>> CREATE TABLE
>>> test=> INSERT INTO test VALUES (1);
>>> INSERT 16392 1
>>> test=> ALTER TABLE test SET WITHOUT OIDS;
>>> ALTER TABLE
>>> test=> INSERT INTO test VALUES(2);
>>> INSERT 0 1
>>> test=> SELECT oid, * FROM test;
>>> ERROR: COLUMN "oid" does not exist
>>> LINE 1: SELECT oid, * FROM test;
>>>
>>> but it has per-table granularity; the oid value for '1' is still
>>> stored, but is no longer visible; that would not work for the security
>>> value.
>> If we can control the Row-level ACLs via table options, what should be
>> happen when we turn off the feature?
>> IMO, it is natural to ignore Row-level ACLs independent from whether
>> the stored tuple actually has ACLs, or not.
>
> Hmmm, sounds like perhaps a GUC is needed there.

Is it possible to control per-table granuality?
Or, per-table control is not necessary?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei wrote:
> >> CREATE TABLE t (
> >> a int,
> >> b text
> >> ) WITH (ROW_LEVEL_ACL=ON);
> >
> > As I mentioned below, this might not be necessary, meaning that row
> > security is enabled for rows where you set the row security value and
> > does not need to be turned on at create table time.
>
> The reason why I proposed the ROW_LEVEL_ACL option is to reduce storage
> consumption for NULL bitmap. For example, the following INSERT originally
> didn't need NULL bitmap, but the "acl column" is implicitly NULL, so the
> HeapTuple has HEAP_HASNULL flag and NULL bitmap is allocated.
>
> INSERT INTO t VALUES (1, 'aaa');
>
> If it is acceptable, I don't think the option is necessary.

I was hoping we could allow the column to be null based only on the
bitmask without having to use the user-column bitmask but maybe that
isn't possible.

> >> As you noticed, a "conditional system column" can be open to discuss.
> >> If someone switch his binary to SE- version, the tables already defined
> >> in $PGDATA don't have "security_context" system column. SE-PostgreSQL
> >> handles tuples stored within the table as "unlabeled" one, but it does
> >> not allow users to access the attribute due to lack of proper system
> >> column.
> >
> > Right, if the "security_context" always exists, but is null, moving to
> > SE-Linux would allow them to add security without dump/reload.
>
> Yes, but it is not recommendable in generally. For meaningful access
> controls, "unlabeled" objects should be labeled properly. :)

We could get tricky and use the proper default when that column is null
for user-visible queries. Again, I am just throwing out ideas.

> >> If it is represented as NULL bitmaps, it requires a tuple has its null bitmaps
> >> which need 4-bytes at least, so it can make additional storage consumption.
> >> Thus, NULL-bitmap approach is not suitable to represent "security column"
> >> because it has to be defined for any tables. But, it can be suitable for
> >> "acl column".
> >
> > Ah, that is a good point, that if we have "security column" which is
> > usually null then we are requiring the NULL bitmask.
> >
> > I was thinking of having them be system columns and therefore only the
> > bitmask would specify if the value exists or not. I am again thinking
> > of having both columns always exist, making your code clearer and more
> > portable for people going to and leaving SE-Postgres.
>
> The HEAP_HASSECURITY flag looks like a specific purpose NULL-bitmap
> which only shows whether the tuple has security value, or not.

Yep, that was my hope.

> >> The following proposal is my idea:
> >>
> >> - The Row-level ACLs is implemented as a hardcoded feature, not a guest of
> >> PGACE security framework.
> >
> > Yep, good.
> >
> >> - It can be enabled/disabled via table options as:
> >> CREATE TABLE t (
> >> a int,
> >> b text
> >> ) WITH (ROW_LEVEL_ACL=ON);
> >
> > Can we make it just always on? See above.
>
> If we assume users set up Row-level ACLs for specific tables, per-table
> option is meaningful for reduction of NULL-bitmap space in the tuple
> without any NULL-values on general columns.

Right. I was hoping there was a way to have HEAP_HASSECACL control if
the value is present or not.

I sure wish others were adding ideas to this discussion.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

KaiGai Kohei wrote:
> >>> Here is our OID column on some rows but not others:
> >>>
> >>> test=> CREATE TABLE test (x INT) WITH oids;
> >>> CREATE TABLE
> >>> test=> INSERT INTO test VALUES (1);
> >>> INSERT 16392 1
> >>> test=> ALTER TABLE test SET WITHOUT OIDS;
> >>> ALTER TABLE
> >>> test=> INSERT INTO test VALUES(2);
> >>> INSERT 0 1
> >>> test=> SELECT oid, * FROM test;
> >>> ERROR: COLUMN "oid" does not exist
> >>> LINE 1: SELECT oid, * FROM test;
> >>>
> >>> but it has per-table granularity; the oid value for '1' is still
> >>> stored, but is no longer visible; that would not work for the security
> >>> value.
> >> If we can control the Row-level ACLs via table options, what should be
> >> happen when we turn off the feature?
> >> IMO, it is natural to ignore Row-level ACLs independent from whether
> >> the stored tuple actually has ACLs, or not.
> >
> > Hmmm, sounds like perhaps a GUC is needed there.
>
> Is it possible to control per-table granuality?
> Or, per-table control is not necessary?

Well, we have a GUC which controls whether tables get an OID column.
If we do the 'secacl' on a per-table level we should probably have a GUC
which would default to ON for all created tables.

Perhaps someone can chime in here to tell us how hard it would be to
allow system columns to be null and not take any storage space, i.e. use
HeapTupleHeaderData.t_infomask on a per-row basis and not
HeapTupleHeaderData.t_bits.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> KaiGai Kohei wrote:
> > >> CREATE TABLE t (
> > >> a int,
> > >> b text
> > >> ) WITH (ROW_LEVEL_ACL=ON);

Let me outline the simplest API, assuming we are using table-level
granularity for the security columns.

CREATE TABLE would support

WITH (ROWACL = TRUE/FALSE);

for row-level acl and:

WITH (SECEXT = TRUE/FALSE);

for SE-Linux, with 'SECEXTL' standing for SECurity EXTernal or
SECurity_contEXT.

And then in postgresql.conf we would have:

default_with_rowacl

and

default_with_secext

which would control the default value of ROWACL and SECEXT when CREATE
TABLE does not specify these values. This is how OIDs works now.

When SE-Linux is enabled, CREATE TABLE would issue an error if SECEXT
was false. I can't think of a clean way to guarantee that existing
tables have SECEXT though, which means we might need to have a missing
'security_context' column mean default SE-Linux permissions.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Let me outline the simplest API, assuming we are using table-level
> granularity for the security columns.
>
> CREATE TABLE would support
>
> WITH (ROWACL = TRUE/FALSE);
>
> for row-level acl and:
>
> WITH (SECEXT = TRUE/FALSE);
>
> for SE-Linux, with 'SECEXTL' standing for SECurity EXTernal or
> SECurity_contEXT.

FYI, in addition COPY and pg_dump would have to have similar settings.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +