Re: RLS restrictive hook policies

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> In get_row_security_policies():
>
> /*
> * If the only built-in policy is the default-deny one, and hook policies
> * exist, then use the hook policies only and do not apply the
> * default-deny policy. Otherwise, we will apply both sets below.
> */
> if (defaultDeny &&
> (hook_policies_restrictive != NIL || hook_policies_permissive != NIL))
> {
> rowsec_expr = NULL;
> rowsec_with_check_expr = NULL;
> }
>
> So if the only policies that exist are restrictive policies from an
> extension, the default-deny policy is not applied and the restrictive
> policies are applied instead, which is effectively a "default-allow"
> situation with each restrictive policy further limiting access to the
> table.

Right, the idea there being that applying the default-deny would render
the restrictive policies pointless, since nothing would ever be visible,
however..

> I think this is potentially very dangerous when both kinds of policy
> exist. Consider for example a situation where initially multiple
> permissive policies are set up for different users allowing them
> access to different subsets of the table, and users not covered by any
> of those permissive policies have no access. Then suppose that later a
> restrictive policy is added, applying to all users -- the intention
> being to prevent any user from accessing some particularly sensitive
> data. The result of adding that restrictive policy would be that all
> the users who previously had no access at all would suddenly have
> access to everything not prohibited by the restrictive policy.
>
> That goes against everything I would expect from a restrictive policy
> -- adding more restrictive policies should only ever reduce the number
> of rows visible, not ever open up more. So I think the test above
> should only disable the default-deny policy if there are any
> permissive policies from the extension.
>
> Of course that will make life a little harder for people who only want
> to use restrictive policies, since they will be forced to first add a
> permissive policy, possibly just one that's always true, but I think
> it's the safest approach.

Requiring a permissive policy which allows the records first, to avoid
the default-deny, makes a ton of sense.

> Possibly if/when we add proper SQL support for defining restrictive
> policies, we might also add an option to ALTER TABLE ... ENABLE ROW
> LEVEL SECURITY to allow a table to have RLS enabled in default-allow
> mode, for users who know they only want to add restrictive policies.

Perhaps... I'm not sure that's really better than simply requiring a
'create policy p1 on t1 using (true);' to be done.

> However, I think that should be something the user has to explicitly
> ask for, not an automatic decision based on the existence of
> restrictive policies.

Agreed. I'm happy to commit that change and back-patch it to 9.5,
barring objections. Given that the only way to have restrictive
policies currently is using hooks, I don't believe there's any
documentation update required either; of course, I'll update the comment
to reflect this discussion/decision and make any necessary changes to
test_rls_hooks.

Thanks!

Stephen