Обсуждение: 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
Re: [HACKERS] [BUGS] BUG #14682: row level security not work with partitioned table
От
Robert Haas
Дата:
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
Re: [HACKERS] [BUGS] BUG #14682: row level security not work with partitioned table
От
Mike Palmiotto
Дата:
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
Re: [HACKERS] [BUGS] BUG #14682: row level security not work with partitioned table
От
Mike Palmiotto
Дата:
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
Re: [HACKERS] [BUGS] BUG #14682: row level security not work with partitioned table
От
Michael Paquier
Дата:
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
Re: [HACKERS] [BUGS] BUG #14682: row level security not work with partitioned table
От
Mike Palmiotto
Дата:
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
Re: [HACKERS] [BUGS] BUG #14682: row level security not work with partitioned table
От
Mike Palmiotto
Дата:
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