Re: Review of Row Level Security

Thanks for your reviewing in spite of large number of lines.

My comments are below.

2012/12/4 Simon Riggs <simon(at)2ndquadrant(dot)com>:
> 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?
>
I think, ANALYZE should perform on the raw tables without row-security
policy. Even though statistics are "gray-zone", it is not a complete set
of the raw table contents, so all we can do is just implying the original
from processed statistical values. The situation is similar to iteration of
probe using PK/FK violation. In general, it is called covert channel, and
out of the scope in regular access control mechanism (including MAC).
So, I don't think we have special protection on pg_stat even if row-
security is configured.

> * 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.
>
The above two comments seems me that you are suggesting to apply
checks on both of scanning rows stage (UPDATE case) and modifying
rows stage (INSERT and UPDATE), to prevent touchable rows getting
gone anywhere.
In the previous discussion, it was suggested we can implement this
feature using before-row trigger. However, I love the idea to support
same row-security policy integrated with CHECK constraint; that kills
individual user's operation to define triggers.
One problem is a case when row-security policy contains SubLink node.
I expect it takes a special case handling, however, also guess not hard
to implement so much.
Let me investigate the code around here.

> * TRUNCATE works, and allows you to remove all rows of a table, even
> ones you can't see to run a DELETE on. Er...
>
It was my oversight. My preference is to rewrite TRUNCATE command
with DELETE statement in case when row-security policy is active on
the target table.
In this case, a NOTICE message may be helpful for users not to assume
the table is always empty after the command.

> 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"
>
I think it is flaw of Oracle. :-)
In case when user can define leakable function, it enables to leak contents
of invisible rows at the timing when executor fetch the rows, prior to
modification
stage, even if we allows to configure individual row-security policies
for SELECT
and DELETE or UPDATE commands.
My preference is one policy on a particular table for all the commands.

> * 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 -
>
I think it should be implemented on the next step. It additionally takes
two independent features (1) functionality to inject a column to store
security label at table definition. (2) functionality to assign a default
security label when a new row is inserted.
As Oracle constructs OLS on the top of VPD feature, the base row-
security feature shall be upstreamed first.

> 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.
>
Indeed. It should be updated as:
although mechanism guarantees to evaluate this condition earlier
than any other user given condition without LEAKPROOF flag
(that means qualifier can have side-effects, thus it possibly leaks
rows should be invisible.)

> * Why is pg_rowlevelsec in a separate catalog table?
>
To define dependency towards functions, operators or relations being
referenced with SubLinks. If we store row-security policy within pg_class
catalog, here is no way to distinguish a dependency records due to row-
security policy or others, thus it makes problem when user wants to
replace the row-security policy.
Do you have a good idea? If this problem can be solved, I can prefer
an approach to store the policy within pg_class.

> * 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.
>
OK, I'll update them.

> * psql \d support needed
>
Are you suggesting to print out full qualifiers of row-security?
Or, a mark to indicate whether row-security is configured, or not?

> * Docs need work, but thats OK.
>
I'd like to want some help with native English speakers.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>