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

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