Re: Row security violation error is misleading

Dean,

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> On 7 April 2015 at 16:21, Stephen Frost <sfrost(at)snowman(dot)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.

Thanks!

> 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.

That was certainly unintentional (and wasn't the case at one point.. I
recall specifically checking that), thanks for addressing it.

> 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.

While I agree that we want to include the RLS policies defined against
the table prior to any policies which are added by the hook, I don't
quite follow what you mean by "cannot be used to bypass built-in RLS".
If we allow, as intended, extensions to define their own policies then
it's entirely possible for the extension to return a "allow all" policy,
and I believe that's perfectly fine.

The idea has come up a couple of times to also have "restrictive"
policies and I agree that's an interesting idea and, once implemented,
we would want to allow extensions to define both kinds and make sure
that we apply "restrictive" policies defined in table-level policies
in addition to policies from extensions, but we're not quite there yet.

> 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 ...".

Right, thanks for working through that and providing a fix.

> 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.

Agreed.

> 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.

Urgh. Thanks for the testing and fix for that.

> 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.

Ah, yes, that makes sense.

> 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.

Right.

> 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.

As I mentioned to Kevin, I'm not sure that this is really a useful
distinction. I'm quite curious if other systems provide that
distinction between grant violations and policy violations. If they do
then that would certainly bolster the argument to provide the
distinction in PG.

> 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.

Good point.

Thanks!

Stephen