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

On 28 May 2015 at 08:51, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
>> 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).
>

So I had a go at refactoring the code in rowsecurity.c to simplify the
default-deny policy handling, as described. In the end my changes were
quite extensive, so I'll understand if you don't have time to review
it, but I think that it's worth it for the improved code clarity,
simplicity and more detailed error messages for restrictive policies.
In any case, there are a couple of bug fixes in there that ought to be
considered.

The main changes are:

* pull_row_security_policies() is replaced by
get_policies_for_relation(), whose remit is to fetch both the internal
and external policies that apply to the relation. It returns the
permissive and restrictive policies as separate lists, each of which
contains any internal policies followed by any external policies
(except of course that internal restrictive policies aren't possible
yet). Unlike pull_row_security_policies() this does not try to build a
default-deny policy, and instead may return empty lists. All the
returned policies are filtered according to the current role, thus
fixing the bug that external policies weren't being filtered.

* process_policies() is replaced by build_security_quals() and
build_with_check_options(), whose remits are to build the lists of
security quals and WithCheckOption checks from the lists of permissive
and restrictive policies. These new functions now have sole
responsibility for handling the default-deny case if there are no
explicit policies to apply, which means that it is no longer necessary
to build RowSecurityPolicy objects representing the default-deny case
(hence no more InvalidOid policy_id).

* get_row_security_policies() is now greatly simplified, since it no
longer has to merge internal and external policies, or worry about
default-deny policies. The guts of the work is now done by the new
functions described above.

* The way that ON CONFLICT DO UPDATE is handled is also simplified ---
rather than recursively calling get_row_security_policies() and
turning the returned list of security quals into WCOs, it now simply
calls build_with_check_options() a couple more times to build the
additional kinds of WCOs needed for the auxiliary UPDATE.

* RelationBuildRowSecurity() no longer builds a default-deny policy,
and the resulting policy list is now allowed to be empty. This removes
the need for RowSecurityPolicy's policy_id field. Actually the old
code had 3 separate places with default-deny policy handling. That is
now all localised in the functions that build security quals and WCOs
from the policy lists.

* Finally, WCOs for restrictive policies now report the name of the
policy violated. Of course this means that the actual error might
depend on the order in which the policies are checked. I've tackled
that in the same way as was recently used for CHECK constraints, which
is to always check restrictive policies in a well-defined order (name
order) so that self-test output is predictable.

Overall, I think this reduces the code complexity (although I think
the total line count is about the same), and there is now a clearer
separation of concerns across the various functions. Also I think it
will make it easier to add support for internal restrictive policies
in the future.

While going through this, I spotted another issue --- in a DML query
with additional non-target relations, such as UPDATE t1 .. FROM t2 ..,
the old code was checking the UPDATE policies of both t1 and t2, but
really I think it ought to be checking the SELECT policies of t2 (in
the same way as this query requires SELECT table permissions on t2,
not UPDATE permissions). I've changed that and added new regression
tests to test that change.

Regards,
Dean