Re: RLS Design

Adam, all,

* Brightwell, Adam (adam.brightwell@crunchydatasolutions.com) wrote:
> Attached is a patch for RLS that was create against master at
> 01363beae52700c7425cb2d2452177133dad3e93 and is ready for review.

Many thanks for posting this.  As others may realize already, I've
reviewed and modified this patch already, working with Adam to get it
ready.  I'm continuing to review and test it, but in general I'm quite
happy with how it's shaping up- additional review, testing, and comments
are always appreciated though.

> Alter a RLS policy named <name> on <table>.  Specifying a command is
> optional, if provided then the policy's command is changed otherwise it is
> left as-is.  Specifying a role is optional, if provided then the policy's
> role is changed otherwise it is left as-is.  The <condition> must always be
> provided and is therefore always replaced.

I'm pretty sure the <condition> is also optional in this patch (that was
a late change that I made), but the documentation needs to be updated.

> * Plancache Invalidation:  If a relation has a row-security policy and
> row-security is enabled then the invalidation will occur when either the
> row_security GUC is changed OR when a the current user changes.  This
> invalidation ONLY takes place for cached plans where the target relation
> has a row security policy.

I know there was a lot of discussion about this previously, but I'm fine
with the initial version simply invalidating plans which involve
RLS-enabled relations and role changes.  This patch doesn't create any
regressions for individuals who are not using RLS.  We can certainly
look into improving this in the future to have per-role plan caches but
it's a fair bit of additional non-trivial code that can be added
independently.

> * Security Qual Expression:  All row-security policies are OR'ed together.

This was also a point of much discussion, but I continue to feel this is
the right approach for the initial version.  We can add flexability here
later, if necessary, but OR'ing these together is in-line with how role
membership works today (you have right for all roles you are a member
of, persuant to inherit/noinherit status, of course).

> * row_security GUC - enable/disable row level security.

Note that, as discussed, pg_dump will set row_security off, unless
specifically asked to enable it.  row_security will also be set to off
when the user logging in is a superuser or does a 'set role' to a
superuser.  Currently, if a user logging in is *not* a superuser, or a
'set role' is done to a non-superuser, row_security gets re-set to
enabled.  This is one aspect of the patch that I think we should change
(which is a matter of removing just a few lines of code and then
updating the regression tests to do 'set row_security = on;' before
running), because if you log in as a superuser and then 'set role' to a
non-superuser, it occurs to me now (it didn't really when I wrote this
originally) as a bit surprising that row_security gets set to 'on' when
doing a 'set role'.

One thing that I really like about this approach is that a superuser can
explicitly set 'row_security' to on and be able to see what happens.
Clearly, in an environment of untrusted users, that could be dangerous,
but it can also be an extremely useful way of testing things,
particularly in development environments where everyone is a superuser.

This deserves a bit more documentation also.

> * BYPASSRLS and NOBYPASSRLS role attribute - allows user to bypass RLS if
> row_security GUC is set to OFF.  If a user sets row_security to OFF and
> does not have this attribute, then an error is raised when attempting to
> query a relation with a RLS policy.

(note that the superuser is always considered to have the bypassrls
attribute)

> * psql \d <table> support: psql describe support for listing policy
> information per table.

This works pretty well for me, but we may want to add some indication
that RLS is on a table in the \dp listing.
Thanks!
    Stephen