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

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