Re: Row security violation error is misleading

On 7 April 2015 at 16:21, Stephen Frost <sfrost@snowman.net> wrote:
> Agreed and we actually have a patch from Dean already to address this,
> it's just been waiting on me (with a couple of other ones).  It'd
> certainly be great if you have time to take a look at those, though,
> generally speaking, I feel prety happy about where those are and believe
> they really just need to be reviewed/tested and maybe a bit of word-
> smithing around the docs.
>

The first of those patches [1] has bit-rotted somewhat, due to the
recent changes to the way rowmarks are handled, so here's an updated
version. It wasn't a trivial merge, because commit
cb1ca4d800621dcae67ca6c799006de99fa4f0a5 made a change to
ExecBuildAuxRowMark() without a matching change to
preprocess_targetlist(), and one of the new RLS-with-inheritance tests
fell over that.

This is not a complete review of RLS, but it does fix a number of issues:

1). In prepend_row_security_policies(), defaultDeny was always true,
so if there were any hook policies, the RLS policies on the table
would just get discarded.

2). In prepend_row_security_policies(), I think it is better to have
any table RLS policies applied before any hook policies, so that a
hook cannot be used to bypass built-in RLS.

3). The infinite recursion detection in fireRIRrules() didn't properly
manage the activeRIRs list in the case of WCOs, so it would
incorrectly report infinite recusion if the same relation with RLS
appeared more than once in the rtable, for example "UPDATE t ... FROM
t ...".

4). The RLS expansion code in fireRIRrules() was handling RLS in the
main loop through the rtable. This lead to RTEs being visited twice if
they contained sublink subqueries, which
prepend_row_security_policies() attempted to handle by exiting early
if the RTE already had securityQuals. That didn't work, however, since
if the query involved a security barrier view on top of a table with
RLS, the RTE would already have securityQuals (from the view) by the
time fireRIRrules() was invoked, and so the table's RLS policies would
be ignored. This is most easily fixed in fireRIRrules() by handling
RLS in a separate loop at the end, after dealing with any other
sublink subqueries, thus ensuring that each RTE is only visited once
for RLS expansion.

5). The inheritance planner code didn't correctly handle non-target
relations with RLS, which would get turned into subqueries during
planning. Thus an update of the form "UPDATE t1 ... FROM t2 ..." where
t1 has inheritance and t2 has RLS quals would fail.

6). process_policies() was adding WCOs to non-target relations, which
is unnecessary, and could lead to a lot of wasted time in the rewriter
and the planner. WCOs are only needed on the result relation.


The second patch [2] is the one that is actually relevant to this
thread. This patch is primarily to apply the RLS checks earlier,
before an update is attempted, more like a regular permissions check.
This adds a new enum to classify the kinds of WCO, a side benefit of
which is that it allows different error messages when RLS checks are
violated, as opposed to WITH CHECK OPTIONs on views.

I actually re-used the sql status code 42501 -
ERRCODE_INSUFFICIENT_PRIVILEGE for a RLS check failure because of the
parallel with permissions checks, but I quite like Craig's idea of
inventing a new status code for this, so that it can be more easily
distinguished from a lack of GRANTed privileges.

There's another side benefit to this patch, which is that the new enum
could be extended to include a new kind of WCO for a check of the
USING quals of a RLS UPDATE policy on the update path of an INSERT..ON
CONFLICT UPDATE. As I pointed out on that thread, I think these quals
need to be treated differently from the WITH CHECK quals of a RLS
UPDATE policy, since they ought to apply to different tuples.
Therefore classifying the WCOs by command type is insufficient to
distinguish the 2 cases.

Regards,
Dean


[1] http://www.postgresql.org/message-id/CAEZATCVoaNiy5qLGda4K-nmP7GbJJgpVGucBAtA1ckVm-uWvgg@mail.gmail.com

[2] http://www.postgresql.org/message-id/CAEZATCVz1u3dyjdzXN3F26rxks2BYXDz--_2rTZfRhuU0zfWSw@mail.gmail.com