Re: RLS open items are vague and unactionable - Mailing list pgsql-hackers

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Sep 11, 2015 at 7:33 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > I've updated the page to add more details about the various items,
> > though the only code changes at this point considered 'open' are this
> > refactoring and the question regarding the 'row-level-security disabled'
> > context which I posted a patch for discussion yesterday.
> >
> > Comments and help on these would certainly be welcome, of course.  We're
> > working on a set of documentation updates to hopefully finish up in the
> > next week to add more details about RLS (additional sub-sections,
> > coverage of the issue Peter raised, additional discussion of RETURNING,
> > etc).
>
> Thanks for the updates.  My thoughts:

Certainly, happy to help.

> On RETURNING, it seems like we've got a fairly fundamental problem
> here.  If I understand correctly, the intention of allowing policies
> to be filtered by command type is to allow blind updates and deletes,

That's not correct, no, will clarify below.

> but this behavior means that they are not really blind.  You can
> always use BEGIN/UPDATE-or-DELETE-with-RETURNING/ROLLBACK as a
> substitute for SELECT.  So the only possible thing you can do with the
> ability to filter by command tag that is coherent from a security
> point of view is to make the update and delete predicates *tighter*
> than the select predicate.

The original intention and the primary expected use-case is to allow the
predicates to be tighter, yes.  What we're discussing here is if we want
to allow that flexibility at all or force users to have a single
visibility policy for all commands.

It's already possible to configure RLS with one visibility policy for
all commands- simply only create a USING clause for 'ALL' commands and
you're done.

The only reason to avoid providing that flexibility is the concern that
it might be misunderstood and users might misconfigure their system.
Removing the flexibility to have per-command visibility policies and
instead force a single visibility policy doesn't add any capabilities.

Further, from the discussion which Dean and I had over the summer and, I
believe, agreed on is that providing *both* a per-command visibility and
having an "overall" visibiility policy (as proposed nearby, where SELECT
always applies but then the other per-command policies are combined with
that policy) ends up being more difficult to reason about and explain
and simply doesn't add any new functionality.

> And if that's where we end up, then haven't we fundamentally
> mis-designed the feature?  I mean, without the blind update case, it
> just seems kooky to have different commands see different rows.  It
> would be better to have all the command see the same rows, and then
> have update/delete *error out* if you try to update a row you're not
> allowed to touch.

Having an error-out option which is based on the values of the existing
rows, instead of only allowing the error-out case based on the values of
the row being added to the relation, is certainly an interesting idea.

As being discussed nearby with Zhaomo (thread 'CREATE POLICY and
RETURNING', which ends up being a pretty bad subject, unfortunately), if
the WITH CHECK option supported referring to 'OLD' and 'NEW' then that
could be supported.  I'm not against having that capability, but it
seems like a new feature rather than an issue with the current
implementation.  That would also imply adding a 'WITH CHECK' clause to
DELETE, to support the "error-instead" option, but that also looks like
a new feature and one which could certainly be added later without any
backwards compatibility concerns, as far as I can see, rather than a
current issue.

Perhaps we would even allow such a 'WITH CHECK' to be applied to SELECT
statements to cause an error to be returned instead, though that would
imply that the visibility policy allows users to query records which
would end up just erroring out due to the 'WITH CHECK' policy and that
might end up providing the user with information about what's in the
relation that they aren't allowed to see due to the 'WITH CHECK' policy.
I can see how it would be useful when it's the intent of the
administrators to produce errors in cases where a user is trying to
access data they are not allowed to, as that could then be audited.  In
such a case, the actual visibility rule might be simply 'true', but an
error is thrown if the rows actually returned do not pass the
'WITH CHECK' policy specified.

> On Dean's refactoring patch, I would tend to favor back-patching
> whatever do there to 9.5, but I'm not volunteering to do the work.

Alright, I'll see about getting that done then.

Thanks!

Stephen