Re: [COMMITTERS] pgsql: Row-Level Security Policies (RLS)

On 27 May 2015 at 14:51, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> On 27 May 2015 at 02:42, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > Now, looking at the code, I'm actually failing to see a case where we
>> > use the RowSecurityPolicy->policy_name.. Perhaps *that's* what we
>> > should be looking to remove?
>>
>> If we add support for restrictive policies, it would be possible, and
>> I think desirable, to report on which policy was violated. For that,
>> having the policy name would be handy. We might also arguably decide
>> to enforce restrictive RLS policies in name order, like check
>> constraints. Of course none of that means it must be kept now, but it
>> feels like a useful field to keep nonetheless.
>
> I agree that it could be useful, but we really shouldn't have fields in
> the current code base which are "just in case".. The one exception
> which comes to mind is if we should keep it for use by extensions.
> Those operate on an independent cycle from our major releases and would
> likely find having the name field useful.
>

Hmm, when I wrote that I had forgotten that we already allow
extensions to add restrictive policies. I think it would be good to
allow those policies to have names, and if they do, to copy those
names to any WCOs created. Then, if a WCO is violated, and it has a
non-NULL policy name associated with it, we should include that in the
error report.

> One thing which now occurs to me, however, is that, while the current
> coding is fine, using InvalidOid as an indicator for the default-deny
> policy, in general, does fall over when we consider policies added by
> extensions. Those policies are necessairly going to need to put
> InvalidOid into that field, unless they acquire an OID somehow
> themselves (it doesn't seem reasonable to make that a requirement,
> though I suppose we could..), and, therefore, perhaps we should add a
> boolean field which indicates which is the defaultDeny policy anyway and
> not use that field for that purpose.
>
> Thoughts?
>

Actually I think a new boolean field is unnecessary, and probably the
policy_id field is too. Re-reading the code in rowsecurity.c, I think
it could use a bit of refactoring. Essentially what it needs to do
(for permissive policies at least) is just

* fetch a list of internal policies
* fetch a list of external policies
* concatenate them
* if the resulting list is empty, add a default-deny qual and/or WCO

By only building the default-deny qual/WCO at the end, rather than
half-way through and pretending that it's a fully-fledged policy, the
code ought to be simpler.

I might get some time at the weekend, so I can take a look and see if
refactoring it this way works out in practice.

BTW, I just spotted another problem with the current code, which is
that policies from extensions aren't being checked against the current
role (policy->roles is just being ignored). I think it would be neater
and safer to move the check_role_for_policy() check up and apply it to
the entire concatenated list of policies, rather than having the lower
level code have to worry about that.

Regards,
Dean