Review of Row Level Security - Mailing list pgsql-hackers

Patch looks good and also like it will/can be ready for 9.3. I'm happy
to put time into this as committer and/or reviewer and take further
responsibility for it, unless others wish to.

LIKES

* It's pretty simple to understand and use

* Check qual is stored in pre-parsed form. That is fast, and also
secure, since changing search_path of the user doesn't change the
security check at all. Nice.

* Performance overhead is low, integration with indexing is clear and
effective and it works with partitioning

* It works, apart from design holes listed below, easily plugged IMHO


DISLIKEs

* Who gets to see stats on the underlying table? Are the stats
protected by security? How does ANALYZE work?

* INSERT ignores the SECURITY clause, on the ground that this has no
meaning. So its possible to INSERT data you can't see. For example,
you can insert medical records for *another* patient, or insert new
top secret information. This also causes a security hole... since
inserted rows can violate defined constraints, letting you know that
other keys you can't see exist. Why don't we treat the SECURITY clause
as a CHECK constraint? That makes intuitive sense to me.

* UPDATE allows you to bypass the SECURITY clause, to produce new rows
that you can't see. (Not good). But you can't get them back again, cos
you can't see them.

* TRUNCATE works, and allows you to remove all rows of a table, even
ones you can't see to run a DELETE on. Er...

None of those things are cool, at all.

Oracle defaults to putting VPD on all event types: INSERT, UPDATE,
DELETE, SELECT. ISTM we should be doing the same, not just say "we can
add an INSERT trigger if you want".

Adding a trigger just begs the question as to why we are bothering in
the first place, since this functionality could already be added by
INSERT, UPDATE or DELETE triggers, if they are a full replacement for
this feature. The only answer is "ease of use"

We can easily add syntax like this

[ROW SECURITY CHECK ( .... ) [ON [ ALL | INSERT, UPDATE, DELETE, SELECT [..,]]]]

with the default being "ALL"

* The design has nothing at all to do with SECURITY LABELs. Why did we
create them, I wonder? I understood we would have row-level label
security. Doesn't that require us to have a data type, such as
reglabel or similar enum? Seems strange. Oracle has two features:
Oracle Label Security and Row Level Security -

OTHER

* The docs should explain a little better how to optimize using RLS.
Specifically, the fact that indexable operators are marked leakproof
and thus can be optimized ahead of the rlsquals. The docs say "rls
quals are guaranteed to be applied first" which isn't true in all
cases.

* Why is pg_rowlevelsec in a separate catalog table?

* Internally, I think we should call this "rowsecurity" rather than
"rowlevelsec" - the "level" word is just noise, whereas the "security"
word benefits from being spelt out in full.

* psql \d support needed

* Docs need work, but thats OK.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services