Sorry for long description again.
Peter Eisentraut wrote:
I have re-reviewed the SE-PostgreSQL patch set. I don't want to talk
about here whether the security model is appropriate, how foreign keys
are to be handled etc. I want to discuss that I really don't like the
architecture of this patch. I have said this before in previous review
rounds, but let me make it a little clearer here. Steps to get your
patch accepted:
One feature at a time
---------------------
By my count, your patch set implements at least three or four major
features:
1a. System-wide consistency in access control
1b. Mandatory access control
2. Row-level security
3. Additional privileges (permission to alter tables, modify large
objects, etc.)
You may object and say, these morally belong together in a
proper/professional/adequate implementation of this feature you have
planned. But realistically, they can be separated. And if a feature is
controversial, difficult, or complicated, it would be in your interest
to deal with one feature at a time. "Deal" means the whole round:
discuss design, write patch, review, test, commit, relax.
I can't afford not to make clear these issues.
In this case, (1a) and (1b) are indivisible, because I want to apply
SELinux as a security server of (1a), SELinux has to be MAC feature.
However, I don't deny a (1b) without (1a) feature like "Oracle Label
Security", which is not a facility I want to make.
I guess (1b) also contains a feature to manage security label.
Please note that I don't really want to (1b) only feature.
The system-wide consistency in access control is the soul.
We can consider (2) as a separated issue. In fact, I already provide
a GUC: sepostgresql_row_level=on/off. It also means whether users
accept a set of benefit(row-level granularity) and demerit(cover
channel of PK/FK), or not.
OK, I don't discuss about covert channel here.
The (3) is involved to (1b). As a basic assumption, MAC system
need to check *any actions* come from clients, even if they have
superuser privileges.
We already have SQL-level privileges, like ACL_SELECT and so on.
However, some of operations implicitly assume request come from
superuser is safe. For example, superuser is allowed to load
a discretionary shared library file, but it also means he is
trusted.
If security design (which is defined by (1a) and (1b) primarily)
does not allow unconditionally trusted user, it is quite natural
to apply additional privileges, even if vanilla one unconditionally
trust superuser.
Unfortunatelly, we don't have any access controls on large object,
but SELinux does not want to provide an information store without
mandatory access controls. So, SE-PostgreSQL applies its access
controls on large object. Thus, (3) is also indivisible from (1a).
If I had to do this, I would first write a patch for #1: A patch that
additionally executes existing privilege checks against an SELinux
policy. Existing privilege checks are a well-defined set: they mostly
happen through pg_xxx_aclcheck() functions. Hook your checks in there.
I had a concern about pg_xxx_aclcheck().
When we accesses a table via a view, pg_xxx_aclcheck() checkes view's
ACLs and table's ACLs are left for unchecked, even if it accesses
same object.
SELinux always makes its decision based on the attribute of object
itself. In other word, the route to access taget object does not
make any affect on its decision making.
For example, a filesystem object (inode) can have multiple pathname
using hard link on operating system. SELinux always makes ite decision
based on inode's attribute (called as security context), independent
from its pathname. Do you know a previous hot discussion between
SELinux and AppArmor in Linux developers?
Please note that I don't deny the benefit of view. It has various
effective usages so we cannot ignore them.
However, it mismatched with the security design, so I needed to put
security hooks on different strategic points.
Row-level security should also be developed as a completely separate
feature, without any SELinux tie-in initially. This is not only
important to make review and verification simpler, but also because we
really need a wider test community for such a tricky feature. And the
set of SELinux users is quite limited, and the intersection with
PostgreSQL developers is almost empty. This was already previously
discussed at length.
It would be possible.
However, note that we should not implement the first step ignoring
upcoming facility.
Eventually I would implement "SE-PostgreSQL 1st edition" with
paying attention for the upcoming row level security.
No in-code frameworks
---------------------
Write your code so that it is fully integrated with the existing code.
Or write a plugin interface and then write a plugin. But don't invent a
"framework" because you are afraid to integrate the new code with the
old code.
At least, PGACE is not a purpose for me.
If fully integrated archtencture is absolutely necessary,
I'll scrap PGACE framework.
Trusted Solaris folks may concern about it, but I cannot give
them higher priority than SE-PostgreSQL getting merged.
(Might sound insensitive, I don't have leeway now.)
The very earlier SE-PostgreSQL put its code directly, because
I didn't pay mention for T-Sol folks at that time.
However...
As mentioned above, permission checks are done through pg_xxx_aclcheck()
functions, which is enough of a framework. I wouldn't want yet another
framework that does more permission checking at other times and places.
If the existing interfaces are not adequate for your purpose, by all
means, extend, refactor, or rewrite them. But don't just avoid it
because you don't want to interfere with the existing code. So scrap
the whole "PGACE" thing.
However, it is a different issue whether pg_xxx_aclcheck() can work as
an alternative of PGACE, or not.
SE-PostgreSQL want to make its decision more than pg_xxx_aclcheck(),
so, in finally, we need to put a code to check on different strategic
points where currently pgaceXXXX() are deployed.
If you need to refactor the aclcheck interfaces, that's another separate
patch, which can easily be reviewed and verified, simplifying the
following patches even further.
I believe a basic premise is vanilla PostgreSQL keeps correct behavior
based on SQL standard. An enhanced security should apply its access
controls orthogonally.
So, I cannot believe refactoring pg_xxx_aclcheck() is not acceptable.
If vanilla PostgreSQL become to check ACLs on tables, independent
from views, do you think it is acceptable?
These things are not going to get done within two weeks.
No wonder!
However, we have to make clear whether the PGACE architecture
is incorrect, or not, at first.
I think the name of PGACE is not important, but it is necessary
to make SELinux's decision in similar strategic point in finally.
Thanks,
--
KaiGai Kohei <kai...@kaigai.gr.jp>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
