Обсуждение: Re: [HACKERS] BUG #14682: row level security not work with partitioned table

On Fri, Jun 02, 2017 at 09:28:16AM +0900, Michael Paquier wrote:
> On Thu, Jun 1, 2017 at 11:13 AM, Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> > This is indeed a bug. fireRIRrules is currently skipping the RLS
> > policy check when relkind == PARTITIONED_TABLES, so RLS policies are
> > not applied. The attached patch fixes the behavior.
> 
> I would expect RLS to trigger as well in this context. Note that there
> should be regression tests to avoid this failure again in the future.
> I have added an open item.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

On 06/04/2017 03:33 PM, Noah Misch wrote:
> On Fri, Jun 02, 2017 at 09:28:16AM +0900, Michael Paquier wrote:
>> On Thu, Jun 1, 2017 at 11:13 AM, Mike Palmiotto
>> <mike.palmiotto@crunchydata.com> wrote:
>> > This is indeed a bug. fireRIRrules is currently skipping the RLS
>> > policy check when relkind == PARTITIONED_TABLES, so RLS policies are
>> > not applied. The attached patch fixes the behavior.
>>
>> I would expect RLS to trigger as well in this context. Note that there
>> should be regression tests to avoid this failure again in the future.
>> I have added an open item.
>
> [Action required within three days.  This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 10 open item.  Robert,
> since you committed the patch believed to have created it, you own this open
> item.

Unless Robert objects, I'll work with Mike to get a fix posted and
committed in the next day or two.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway <mail@joeconway.com> wrote:
> Unless Robert objects, I'll work with Mike to get a fix posted and
> committed in the next day or two.

That would be great.  Thanks.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway <mail@joeconway.com> wrote:
>> Unless Robert objects, I'll work with Mike to get a fix posted and
>> committed in the next day or two.
>
> That would be great.  Thanks.

I have the updated patch with rowsecurity regression tests and rebased
on master. I've run these and verified locally by feeding
rowsecurity.sql to psql, but have yet to get the full regression suite
passing -- it's failing on the constraints regtest and then gets stuck
in recovery. Undoubtedly something to do with my
configuration/environment over here. I'm working through those issues
right now. In the meantime, if you want to see the regression tests as
they stand, please see the attached patch.

Thanks,

-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/06/2017 11:57 AM, Mike Palmiotto wrote:
> On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway <mail@joeconway.com> wrote:
>>> Unless Robert objects, I'll work with Mike to get a fix posted and
>>> committed in the next day or two.
>>
>> That would be great.  Thanks.
>
> I have the updated patch with rowsecurity regression tests and rebased
> on master. I've run these and verified locally by feeding
> rowsecurity.sql to psql, but have yet to get the full regression suite
> passing -- it's failing on the constraints regtest and then gets stuck
> in recovery. Undoubtedly something to do with my
> configuration/environment over here. I'm working through those issues
> right now. In the meantime, if you want to see the regression tests as
> they stand, please see the attached patch.

The constraints test passes here, so presumably something you borked
locally. I only see a rowsecurity failure, which is not surprising since
your patch does not include the changes to expected output ;-)
Please resend with src/test/regress/expected/rowsecurity.out included.

Thanks,

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Tue, Jun 6, 2017 at 4:07 PM, Joe Conway <mail@joeconway.com> wrote:
> On 06/06/2017 11:57 AM, Mike Palmiotto wrote:
>> On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway <mail@joeconway.com> wrote:
>>>> Unless Robert objects, I'll work with Mike to get a fix posted and
>>>> committed in the next day or two.
>>>
>>> That would be great.  Thanks.
>>
>> I have the updated patch with rowsecurity regression tests and rebased
>> on master. I've run these and verified locally by feeding
>> rowsecurity.sql to psql, but have yet to get the full regression suite
>> passing -- it's failing on the constraints regtest and then gets stuck
>> in recovery. Undoubtedly something to do with my
>> configuration/environment over here. I'm working through those issues
>> right now. In the meantime, if you want to see the regression tests as
>> they stand, please see the attached patch.
>
> The constraints test passes here, so presumably something you borked
> locally. I only see a rowsecurity failure, which is not surprising since
> your patch does not include the changes to expected output ;-)
> Please resend with src/test/regress/expected/rowsecurity.out included.

It was indeed an issue on my end. Attached are the rowsecurity
regression tests and the expected out. Unsurprisingly, all tests pass,
because I said so. :)

Let me know if you want me to make any revisions.

Thanks,

--
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/06/2017 02:44 PM, Mike Palmiotto wrote:
> On Tue, Jun 6, 2017 at 4:07 PM, Joe Conway <mail@joeconway.com> wrote:
>> On 06/06/2017 11:57 AM, Mike Palmiotto wrote:
>>> On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway <mail@joeconway.com> wrote:
>>>>> Unless Robert objects, I'll work with Mike to get a fix posted and
>>>>> committed in the next day or two.
>>>>
>>>> That would be great.  Thanks.
>>>
>>> I have the updated patch with rowsecurity regression tests and rebased
>>> on master. I've run these and verified locally by feeding
>>> rowsecurity.sql to psql, but have yet to get the full regression suite
>>> passing -- it's failing on the constraints regtest and then gets stuck
>>> in recovery. Undoubtedly something to do with my
>>> configuration/environment over here. I'm working through those issues
>>> right now. In the meantime, if you want to see the regression tests as
>>> they stand, please see the attached patch.
>>
>> The constraints test passes here, so presumably something you borked
>> locally. I only see a rowsecurity failure, which is not surprising since
>> your patch does not include the changes to expected output ;-)
>> Please resend with src/test/regress/expected/rowsecurity.out included.
>
> It was indeed an issue on my end. Attached are the rowsecurity
> regression tests and the expected out. Unsurprisingly, all tests pass,
> because I said so. :)
>
> Let me know if you want me to make any revisions.


Thanks Mike. I'll take a close look to verify output correctnes, but I
am concerned that the new tests are unnecessarily complex. Any other
opinions on that?

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Wed, Jun 7, 2017 at 9:52 AM, Joe Conway <mail@joeconway.com> wrote:
> On 06/06/2017 02:44 PM, Mike Palmiotto wrote:
>> On Tue, Jun 6, 2017 at 4:07 PM, Joe Conway <mail@joeconway.com> wrote:
>>> On 06/06/2017 11:57 AM, Mike Palmiotto wrote:
>>>> On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>>> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway <mail@joeconway.com> wrote:
>>>>>> Unless Robert objects, I'll work with Mike to get a fix posted and
>>>>>> committed in the next day or two.
>>>>>
>>>>> That would be great.  Thanks.
>>>>
>>>> I have the updated patch with rowsecurity regression tests and rebased
>>>> on master. I've run these and verified locally by feeding
>>>> rowsecurity.sql to psql, but have yet to get the full regression suite
>>>> passing -- it's failing on the constraints regtest and then gets stuck
>>>> in recovery. Undoubtedly something to do with my
>>>> configuration/environment over here. I'm working through those issues
>>>> right now. In the meantime, if you want to see the regression tests as
>>>> they stand, please see the attached patch.
>>>
>>> The constraints test passes here, so presumably something you borked
>>> locally. I only see a rowsecurity failure, which is not surprising since
>>> your patch does not include the changes to expected output ;-)
>>> Please resend with src/test/regress/expected/rowsecurity.out included.
>>
>> It was indeed an issue on my end. Attached are the rowsecurity
>> regression tests and the expected out. Unsurprisingly, all tests pass,
>> because I said so. :)
>>
>> Let me know if you want me to make any revisions.
>
>
> Thanks Mike. I'll take a close look to verify output correctnes, but I
> am concerned that the new tests are unnecessarily complex. Any other
> opinions on that?

Some tests would be good to have. Now, if I read those regression
tests correctly, this is using 10 relations where two would be enough,
one as the parent relation and one as a partition. Then policies apply
on the parent relation. The same kind of policy is defined 4 times,
and there is bloat with GRANT and ALTER TABLE commands.

+SELECT * FROM part_document;
+ did | cid | dlevel |      dauthor      |         dtitle
+-----+-----+--------+-------------------+-------------------------
+   1 |  11 |      1 | regress_rls_bob   | my first novel
Adding an "ORDER BY did" as well here would make the test output more
predictable.
-- 
Michael

On Tue, Jun 6, 2017 at 9:12 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Jun 7, 2017 at 9:52 AM, Joe Conway <mail@joeconway.com> wrote:
>> Thanks Mike. I'll take a close look to verify output correctnes, but I
>> am concerned that the new tests are unnecessarily complex. Any other
>> opinions on that?
>
> Some tests would be good to have. Now, if I read those regression
> tests correctly, this is using 10 relations where two would be enough,
> one as the parent relation and one as a partition. Then policies apply
> on the parent relation. The same kind of policy is defined 4 times,
> and there is bloat with GRANT and ALTER TABLE commands.

I ended up narrowing it down to 4 tables (one parent and 3 partitions)
in order to demonstrate policy sorting and order of RLS/partition
constraint checking. It should be much more straight-forward now, but
let me know if there are any further recommended changes.

One thing that concerns me is the first EXPLAIN plan from regress_rls_dave:
+EXPLAIN (COSTS OFF) SELECT * FROM part_document WHERE f_leak(dtitle);
+                                             QUERY PLAN
+----------------------------------------------------------------------------------------------------
+ Append
+   InitPlan 1 (returns $0)
+     ->  Index Scan using uaccount_pkey on uaccount
+           Index Cond: (pguser = CURRENT_USER)
+   ->  Seq Scan on part_document_fiction
+         Filter: ((cid <> 55) AND (cid <> 55) AND (cid < 99) AND
(dlevel <= $0) AND f_leak(dtitle))
+   ->  Seq Scan on part_document_satire
+         Filter: ((cid <> 55) AND (cid <> 55) AND (cid < 99) AND
(dlevel <= $0) AND f_leak(dtitle))
+(8 rows)

I would expect that both part_document_satire (cid == 55) and
part_document_nonfiction (cid == 99) would be excluded from the
explain, but only cid < 99 seems to work. Interestingly, when I change
policy pp1r to cid < 55, I see the following:

+EXPLAIN (COSTS OFF) SELECT * FROM part_document WHERE f_leak(dtitle);
+                                            QUERY PLAN
+---------------------------------------------------------------------------------------------------
+ Append
+   InitPlan 1 (returns $0)
+     ->  Index Scan using uaccount_pkey on uaccount
+           Index Cond: (pguser = CURRENT_USER)
+   ->  Seq Scan on part_document_fiction
+         Filter: ((cid < 55) AND (cid <> 55) AND (cid < 99) AND
(dlevel <= $0) AND f_leak(dtitle))
+(6 rows)

Is this a demonstration of a non-immutable function backing the
operator and thus not being able to filter it from the planner, or is
it a bug?

>
> +SELECT * FROM part_document;
> + did | cid | dlevel |      dauthor      |         dtitle
> +-----+-----+--------+-------------------+-------------------------
> +   1 |  11 |      1 | regress_rls_bob   | my first novel
> Adding an "ORDER BY did" as well here would make the test output more
> predictable.

Done.

Thanks,
-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
> I ended up narrowing it down to 4 tables (one parent and 3 partitions)
> in order to demonstrate policy sorting and order of RLS/partition
> constraint checking. It should be much more straight-forward now, but
> let me know if there are any further recommended changes.

Thanks, will take a look towards the end of the day.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Wed, Jun 7, 2017 at 9:49 AM, Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
> One thing that concerns me is the first EXPLAIN plan from regress_rls_dave:
> +EXPLAIN (COSTS OFF) SELECT * FROM part_document WHERE f_leak(dtitle);
> +                                             QUERY PLAN
> +----------------------------------------------------------------------------------------------------
> + Append
> +   InitPlan 1 (returns $0)
> +     ->  Index Scan using uaccount_pkey on uaccount
> +           Index Cond: (pguser = CURRENT_USER)
> +   ->  Seq Scan on part_document_fiction
> +         Filter: ((cid <> 55) AND (cid <> 55) AND (cid < 99) AND
> (dlevel <= $0) AND f_leak(dtitle))
> +   ->  Seq Scan on part_document_satire
> +         Filter: ((cid <> 55) AND (cid <> 55) AND (cid < 99) AND
> (dlevel <= $0) AND f_leak(dtitle))
> +(8 rows)
>
> I would expect that both part_document_satire (cid == 55) and
> part_document_nonfiction (cid == 99) would be excluded from the
> explain, but only cid < 99 seems to work. Interestingly, when I change
> policy pp1r to cid < 55, I see the following:
>
> +EXPLAIN (COSTS OFF) SELECT * FROM part_document WHERE f_leak(dtitle);
> +                                            QUERY PLAN
> +---------------------------------------------------------------------------------------------------
> + Append
> +   InitPlan 1 (returns $0)
> +     ->  Index Scan using uaccount_pkey on uaccount
> +           Index Cond: (pguser = CURRENT_USER)
> +   ->  Seq Scan on part_document_fiction
> +         Filter: ((cid < 55) AND (cid <> 55) AND (cid < 99) AND
> (dlevel <= $0) AND f_leak(dtitle))
> +(6 rows)
>
> Is this a demonstration of a non-immutable function backing the
> operator and thus not being able to filter it from the planner, or is
> it a bug?

Assuming my digging is correct, there's some other explanation for
this not working as expected...
postgres=> select po.oprname, pp.proname, pp.provolatile from pg_proc
pp join pg_operator po on pp.proname::text = po.oprcode::text where
po.oprname = '<>' and pp.proname like 'int%ne';oprname |   proname   | provolatile
---------+-------------+-------------<>      | int4ne      | i<>      | int2ne      | i<>      | int24ne     | i<>
|int42ne     | i<>      | int8ne      | i<>      | int84ne     | i<>      | int48ne     | i<>      | interval_ne | i<>
   | int28ne     | i<>      | int82ne     | i
 
(10 rows)

Thoughts?

Thanks,
-- 
Mike Palmiotto
Software Engineer
Crunchy Data Solutions
https://crunchydata.com

On Wed, Jun 07, 2017 at 08:45:20AM -0700, Joe Conway wrote:
> On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
> > I ended up narrowing it down to 4 tables (one parent and 3 partitions)
> > in order to demonstrate policy sorting and order of RLS/partition
> > constraint checking. It should be much more straight-forward now, but
> > let me know if there are any further recommended changes.
> 
> Thanks, will take a look towards the end of the day.

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

On 06/08/2017 11:09 PM, Noah Misch wrote:
> On Wed, Jun 07, 2017 at 08:45:20AM -0700, Joe Conway wrote:
>> On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
>> > I ended up narrowing it down to 4 tables (one parent and 3 partitions)
>> > in order to demonstrate policy sorting and order of RLS/partition
>> > constraint checking. It should be much more straight-forward now, but
>> > let me know if there are any further recommended changes.
>>
>> Thanks, will take a look towards the end of the day.
>
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

I started reviewing the latest patch last night and will try to finish
up this afternoon (west coast USA time).

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On 06/09/2017 06:16 AM, Joe Conway wrote:
> On 06/08/2017 11:09 PM, Noah Misch wrote:
>> On Wed, Jun 07, 2017 at 08:45:20AM -0700, Joe Conway wrote:
>>> On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
>>> > I ended up narrowing it down to 4 tables (one parent and 3 partitions)
>>> > in order to demonstrate policy sorting and order of RLS/partition
>>> > constraint checking. It should be much more straight-forward now, but
>>> > let me know if there are any further recommended changes.
>>>
>>> Thanks, will take a look towards the end of the day.
>>
>> This PostgreSQL 10 open item is past due for your status update.  Kindly send
>> a status update within 24 hours, and include a date for your subsequent status
>> update.  Refer to the policy on open item ownership:
>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>
> I started reviewing the latest patch last night and will try to finish
> up this afternoon (west coast USA time).

I left the actual (2 line) code change untouched, but I tweaked the
regression test changes a bit. If there are no complaints I will push
tomorrow (Saturday).

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On 06/09/2017 02:52 PM, Joe Conway wrote:
> On 06/09/2017 06:16 AM, Joe Conway wrote:
>> On 06/08/2017 11:09 PM, Noah Misch wrote:
>>> On Wed, Jun 07, 2017 at 08:45:20AM -0700, Joe Conway wrote:
>>>> On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
>>>> > I ended up narrowing it down to 4 tables (one parent and 3 partitions)
>>>> > in order to demonstrate policy sorting and order of RLS/partition
>>>> > constraint checking. It should be much more straight-forward now, but
>>>> > let me know if there are any further recommended changes.
>>>>
>>>> Thanks, will take a look towards the end of the day.
>>>
>>> This PostgreSQL 10 open item is past due for your status update.  Kindly send
>>> a status update within 24 hours, and include a date for your subsequent status
>>> update.  Refer to the policy on open item ownership:
>>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>>
>> I started reviewing the latest patch last night and will try to finish
>> up this afternoon (west coast USA time).
>
> I left the actual (2 line) code change untouched, but I tweaked the
> regression test changes a bit. If there are no complaints I will push
> tomorrow (Saturday).

I waited until Sunday, but pushed none-the-less.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development