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.
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.
This patch would already provide useful functionality, but it would be
much easier to review and verify, because we know how permission
checking works in the existing system, so we can compare them. And it
would build confidence among developers and users about the whole idea,
about SELinux integration etc.
Then you can tackle #3: Place permission checks in more places. This
patch would be simple to review and verify, because at this point we
already know how SELinux integration works, and making more use of it is
not such a big step. At this point we could also discuss whether some
of these additional checks are useful enough to also expose via the
traditional SQL ACLs, which would further simplify the patch.
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.
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.
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.
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.
These things are not going to get done within two weeks. But if you
start producing small, self-contained patches along the above lines, you
are much more likely to make progress over the coming development cycle.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
