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

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> 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.

I'm certainly not against that and I agree that it'd be nicer than
simply reporting that there was a violation, but we combine all of the
various pieces together, no? Are we really going to be able to
confidently say which policy was violated? Even if we are able to say
the first which was violated, more than one might be. Is that an issue
we need to address? Perhaps not, but it's something to consider.

> > 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.
>
> 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 tend to agree.

> 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.

That would certainly be fantastic and most appreciated. Beyond that, we
have the add-a-default-deny-policy logic in multiple places, as I
recall, and I wonder if that's overkill and done out of paranoia rather
than sound judgement. I'm certainly not suggesting that we take
unnecessary risks, and so perhaps we should keep it, but I do think it's
something which should be reviewed and considered (I've been planning to
do so for a while, in fact).

> 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.

Excellent point... We should address that and your proposed approach
sounds reasonable to me. If you're able to work on that this weekend,
I'd be happy to review next week.

Thanks!

Stephen