Re: Row security violation error is misleading
От | Dean Rasheed |
---|---|
Тема | Re: Row security violation error is misleading |
Дата | |
Msg-id | CAEZATCVmFUfUOwwhnBTcgi6AquyjQ0-1fyKd0T3xBWJvn+xsFA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Row security violation error is misleading (Stephen Frost <sfrost@snowman.net>) |
Ответы |
Re: Row security violation error is misleading
Re: Row security violation error is misleading Re: Row security violation error is misleading Re: Row security violation error is misleading |
Список | pgsql-hackers |
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
Вложения
В списке pgsql-hackers по дате отправления: