Re: [v9.3] Row-Level Security - Mailing list pgsql-hackers

On Thu, Oct 18, 2012 at 2:19 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Kohei KaiGai escribió:
>> The revised patch fixes the problem that Daen pointed out.
>
> Robert, would you be able to give this latest version of the patch a
> look?

Yeah, sorry I've been completely sidelined this CommitFest. It's been
a crazy couple of months.  Prognosis for future craziness reduction
uncertain.  Comments:

The documentation lists several documented limitations that I would
like to analyze a little bit.  First, it says that row-level security
policies are not applied on UPDATE or DELETE.  That sounds downright
dangerous to me.  Is there some really compelling reason we're not
doing it?  Second, it says that row-level security policies are not
currently applied on INSERT, so you should use a trigger, but implies
that this will change in the future.  I don't think we should change
that in the future; I think relying on triggers for that case is just
fine.  Note that it could be an issue with the post-image for UPDATES,
as well, and I think the trigger solution is similarly adequate to
cover that case.  With respect to the documented limitation regarding
DECLARE/FETCH, what exactly will happen?  Can we describe this a bit
more clearly rather than just saying the behavior will be
unpredictable?

It looks suspiciously as if the row-level security mode needs to be
saved and restored in all the same places we call save and restore the
user ID and security context.  Is there some reason the
row-level-security-enabled flag shouldn't just become another bit in
the security context?  Then we'd get all of this save/restore logic
mostly for free.

ATExecSetRowLevelSecurity() calls SetRowLevelSecurity() or
ResetRowLevelSecurity() to update pg_rowlevelsec, but does the
pg_class update itself.  I think that all of this logic should be
moved into  a single function, or at least functions in the same file,
with the one that only updates pg_rowlevelsec being static and
therefore not able to be called from outside the file.  We always need
the pg_class update and the pg_rowlevelsec update to happen together,
so it's not good to have an exposed function that does one of those
updates but not the other.  I think the simplest thing is just to move
ATExecSetRowLevelSecurity to pg_rowlevelsec.c and rename it to
SetRowLevelSecurity() and then give it two static helper functions,
say InsertPolicyRow() and DeletePolicyRow().

I think it would be good if Tom could review the query-rewriting parts
of this (viz rowlevelsec.c) as I am not terribly familiar with this
machinery, and of course anything we get wrong here will have security
consequences.  At first blush, I'm somewhat concerned about the fact
that we're trying to do this after query rewriting; that seems like it
could break things.  I know KaiGai mentioned upthread that the
rewriter won't be rerun if the plan is invalidated, but (1) why is
that OK now? and (2) if it is OK now, then why is it OK to do
rewriting of the RLS qual - only - after rewriting if all of the rest
of the rewriting needs to happen earlier?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company