Re: [HACKERS] Infinite recursion in row-security based on updatable s.b. views
On 01/28/2014 02:11 PM, Craig Ringer wrote: My first thought is to add a boolean flag to RangeTblEntry (similar to the inh flag) to say whether RLS expansion is requested for that RTE. Then set it to false on each RTE as you add new RLS quals to it's securityQuals. That's what I was getting at with adding flags to RangeTblEntry, yes. Given the number of flags we're growing I wonder if they should be consolodated into a bitmask, but I'll leave that problem for later. I'll do this part first, since it seems you agree that a RangeTblEntry flag is the appropriate path. That'll make row-security checking work and make the patch testable. It won't deal with recursive rules, they'll still crash the backend. I'll deal with that as a further step. I've put together a working RLS patch on top of updatable security barrier views. It has some known issues remaining; it doesn't do recursion checking yet, and it fails some of the regression tests in exciting ways. I'm looking into them step by step. Some differences in the tests behaviours that have changed due to the inheritance rules changing; many appear to be oversights or bugs yet to be chased down. You can find it here; https://github.com/ringerc/postgres/compare/rls-9.4-upd-sb-views i.e. https://github.com/ringerc/postgres.git , branch rls-9.4-upd-sb-views (subject to rebasing) or the non-rebased tag rls-9.4-upd-sb-views-v2 The guts of the patch appear as a diff, attached, but it's not standalone so I suggest using git. I'll be looking into recursion issues and the test failures tomorrow. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services From 03c02389c7b475d06864f9a7f38fd583c6e891e9 Mon Sep 17 00:00:00 2001 From: Craig Ringer cr...@2ndquadrant.com Date: Tue, 28 Jan 2014 14:23:23 +0800 Subject: [PATCH 1/4] RLS: Add rowsec_done flag to RangeTblEntry To be used to track completion status of row security expansion on a range table entry. Rels with this flag set must not be row-security expanded. --- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/include/nodes/parsenodes.h | 1 + 4 files changed, 4 insertions(+) diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 3e2acf0..9607911 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -1975,6 +1975,7 @@ _copyRangeTblEntry(const RangeTblEntry *from) COPY_SCALAR_FIELD(rtekind); COPY_SCALAR_FIELD(relid); COPY_SCALAR_FIELD(relkind); + COPY_SCALAR_FIELD(rowsec_done); COPY_NODE_FIELD(subquery); COPY_SCALAR_FIELD(security_barrier); COPY_SCALAR_FIELD(rowsec_relid); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index dd447ed..974d169 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -2372,6 +2372,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node) case RTE_RELATION: WRITE_OID_FIELD(relid); WRITE_CHAR_FIELD(relkind); + WRITE_BOOL_FIELD(rowsec_done); break; case RTE_SUBQUERY: WRITE_NODE_FIELD(subquery); diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index a0cb369..7a43b59 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -1214,6 +1214,7 @@ _readRangeTblEntry(void) case RTE_RELATION: READ_OID_FIELD(relid); READ_CHAR_FIELD(relkind); + READ_BOOL_FIELD(rowsec_done); break; case RTE_SUBQUERY: READ_NODE_FIELD(subquery); diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index b3e718a..b3ae722 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -736,6 +736,7 @@ typedef struct RangeTblEntry */ Oid relid; /* OID of the relation */ char relkind; /* relation kind (see pg_class.relkind) */ + bool rowsec_done; /* True if row-security quals checked for and applied already */ /* * Fields valid for a subquery RTE (else NULL): -- 1.8.3.1 From b0f5797d81a4b856f26818e14c93a0ec453a35de Mon Sep 17 00:00:00 2001 From: Craig Ringer cr...@2ndquadrant.com Date: Fri, 24 Jan 2014 16:18:40 +0800 Subject: [PATCH 2/4] RLS: Enforce row-security constraints Row-security constraints are enforced here by having the securityQual expansion code check for a row-security constraint before expanding quals. --- src/backend/optimizer/prep/prepsecurity.c | 17 +++- src/backend/optimizer/prep/prepunion.c| 5 ++ src/backend/optimizer/util/Makefile | 3 +- src/backend/optimizer/util/rowsecurity.c | 144 ++ src/include/optimizer/rowsecurity.h | 25 ++ 5 files changed, 192 insertions(+), 2 deletions(-) create mode 100644 src/backend/optimizer/util/rowsecurity.c create mode 100644 src/include/optimizer/rowsecurity.h diff --git a/src/backend/optimizer/prep/prepsecurity.c b/src/backend/optimizer/prep/prepsecurity.c
Re: [HACKERS] Infinite recursion in row-security based on updatable s.b. views
On 01/24/2014 07:16 PM, Dean Rasheed wrote: think recursively calling the rewriter to expand view references in the new RLS qual, and expand_security_qual() to expand any additional RLS quals in the securityQuals list With this, it'd be helpful if expand_security_qual(...) took a RangeTblEntry instead of an rt_index. That'd also be much more efficient with large rtables if we can arrange a scan through the rtable when looking for security quals. Like other places that operate on the rangetable while it's being modified, we can walk the rangetable list up until the final entry that existed when we started walking. This approach saves the series of rt_fetch calls, which are something like O(n log n) for n relations. It's safe because the operation will only append rangetable entries. (I can't help wonder how much we'd gain by making the rtable an array that gets doubled in size and copied whenever it overflows, rather than a linked list, given all the walking of it that gets done, and how dead entries to get flagged as dead rather than actually removed.) I'm looking for where I found the code that already does this so I can point and say I'm not crazy, we already do it here. Will follow up with a patch. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Infinite recursion in row-security based on updatable s.b. views
On 01/28/2014 04:39 PM, Craig Ringer wrote: I'm looking for where I found the code that already does this so I can point and say I'm not crazy, we already do it here. Will follow up with a patch. I spoke to soon. The code I'm talking about is expand_inherited_tables(...) and it still uses rt_fetch, it just avoids foreach(...) in favour of stopping scanning at the end of the original rtable. So I get to be crazy after all. I really don't like how many places we're rt_fetch'ing the same RTE from in updatable s.b. views and its interaction with row-security, but that can be a later problem. For now I'll stick with RTIs. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Infinite recursion in row-security based on updatable s.b. views
On 01/24/2014 07:16 PM, Dean Rasheed wrote: My first thought is to add a boolean flag to RangeTblEntry (similar to the inh flag) to say whether RLS expansion is requested for that RTE. Then set it to false on each RTE as you add new RLS quals to it's securityQuals. That's what I was getting at with adding flags to RangeTblEntry, yes. Given the number of flags we're growing I wonder if they should be consolodated into a bitmask, but I'll leave that problem for later. I'll do this part first, since it seems you agree that a RangeTblEntry flag is the appropriate path. That'll make row-security checking work and make the patch testable. It won't deal with recursive rules, they'll still crash the backend. I'll deal with that as a further step. In addition, when adding RLS quals to an RTE, I think they should be fully and recursively expanded immediately, before setting the new flag to false and moving on --- think recursively calling the rewriter to expand view references in the new RLS qual, and expand_security_qual() to expand any additional RLS quals in the securityQuals list --- with loop detection there. I'm not pretending that's going to be easy, but there are a couple of existing pieces of code to borrow ideas from. Doing it this way should make it possible to do the loop detection without any global structures. Ugh. I was really hoping to avoid *another* place where we do recursive expansion and infinite recursion checking, especially when the rewriter already does this job. Unfortunately, so long as the rewriter doesn't know about inheritance, it cannot fully solve this problem. A mutually recursive rule involving inheritance wouldn't get detected by rewriter based checking. The original RLS patch only detects direct recursion, btw, it looks like it won't catch mutual recursion. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Infinite recursion in row-security based on updatable s.b. views
Hi all I've hit an interesting wrinkle and am interested in opinions. By integrating updatable security barrier view support with row-security, it has become a lot harder to detect and prevent infinite recursion (including mutual recursion) in row-security policy expansion. The code is attached, but it's not an independent patch so it's way easier to grab it from git: g...@github.com:ringerc/postgres.git branch rls-9.4-upd-sb-views (subject to rebase); or tagrls-9.4-upd-sb-views-v1 (Only contains half the old row-security patch; I'll rebase the docs, tests, etc on top of this once it's working properly). I've integrated the updatable security barrier view support into RLS by injecting securityQuals in subquery_planner() just before preprocess_rowmarks . (I'm still thinking about some inheritance related aspects to that, but that's for later). The problem is that this causes infinite recursion - the securityQuals get expanded into a subquery over the original RTE that had the row-security policy on it. Then subquery_planner is re-entered when processing the subquery, a row-security qual is found on the target RTE, and ... *boom*. Fixing this is not as simple as suppressing expansion of row-security policy when processing a security barrier subquery created by a row-security policy, as it is desirable to respect the row-security policy of *other* tables that get referenced in the expanded row-security qual. If we just record the relid of the relation a subquery was expanded from and avoid expanding that inside the generated subquery we prevent simple linear recursion, but not mutual recursion between two or more rels with row-security policy. KaiGai's row-security patch handles this because it does its own recursive expansion, so (like the rewriter) it can keep a breadcrumb trail and detect when it is repeating a path. That's not so practical when row-security code tags RTEs with policy, then updatable s.b. views goes and expands them. So. Options. 1.Come up with a reasonable way to track recursive row-security expansion, detect infinite recursion, and bail out. Likely to involve a new global structure with planner/optimizer lifetime that gets checked and maintained by apply_row_security_rangetbl. 2.Solve the linear recursion case by storing a relid that should not get expanded for security quals when processing a subquery. Say don't do that, expect stack exceeded crashes if you do for the mutual recursion case. Seems user hostile, incomplete, and likely to break people's systems when they really don't expect it. 3.Disregard row-security policy on referenced tables when expanding row-security qualifiers. There's precendent here in foreign keys, which ignore row-security policy, but I don't think this is at all appealing. 4.Magic? Unless someone has a suggestion for #4, I'll be going with #1. I'd appreciate tips on doing this in a sane and efficient manner if anyone has them. I'll be reading over the rewriter's infinite loop protection and that of KaiGai's RLS patch for ideas in the mean time, and will give it a good go. BTW, I feel like we should be letting the rewriter do this job; it's good at dealing with recursion problems already. That won't work so long as security barrier qual expansion happens during planning, not rewrite, though - and we've already explored the fun problems with doing upd. s.b. qual expansion in rewrite. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services From 4f8ed71e4cf18db7a4285da9b9c9f8f7f1030e32 Mon Sep 17 00:00:00 2001 From: Craig Ringer cr...@2ndquadrant.com Date: Fri, 24 Jan 2014 16:18:40 +0800 Subject: [PATCH] RLS: First attempt to integrate with updatable s.b. views Enters infinite recursion because the rls-protected table gets expanded and the new RTE gets checked for row-security quals, gets securityQuals added, and ... infinite recursion! --- src/backend/optimizer/plan/planner.c | 7 ++ src/backend/optimizer/prep/prepunion.c | 6 + src/backend/optimizer/util/Makefile | 3 +- src/backend/optimizer/util/rowsecurity.c | 185 +++ src/include/optimizer/rowsecurity.h | 27 + 5 files changed, 227 insertions(+), 1 deletion(-) create mode 100644 src/backend/optimizer/util/rowsecurity.c create mode 100644 src/include/optimizer/rowsecurity.h diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 80940ea..8cdd580 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -388,6 +388,13 @@ subquery_planner(PlannerGlobal *glob, Query *parse, } /* + * Check RTEs for row-security policies and set securityQuals on the + * RTE if a policy is found. This must happen before inherited table + * expansion so that the quals get copied to the child rels. + */ + apply_row_security_policy(root); + + /* * Preprocess RowMark
Re: [HACKERS] Infinite recursion in row-security based on updatable s.b. views
On 24 January 2014 09:04, Craig Ringer cr...@2ndquadrant.com wrote: Hi all I've hit an interesting wrinkle and am interested in opinions. By integrating updatable security barrier view support with row-security, it has become a lot harder to detect and prevent infinite recursion (including mutual recursion) in row-security policy expansion. The code is attached, but it's not an independent patch so it's way easier to grab it from git: g...@github.com:ringerc/postgres.git branch rls-9.4-upd-sb-views (subject to rebase); or tagrls-9.4-upd-sb-views-v1 (Only contains half the old row-security patch; I'll rebase the docs, tests, etc on top of this once it's working properly). I've integrated the updatable security barrier view support into RLS by injecting securityQuals in subquery_planner() just before preprocess_rowmarks . (I'm still thinking about some inheritance related aspects to that, but that's for later). The problem is that this causes infinite recursion - the securityQuals get expanded into a subquery over the original RTE that had the row-security policy on it. Then subquery_planner is re-entered when processing the subquery, a row-security qual is found on the target RTE, and ... *boom*. Fixing this is not as simple as suppressing expansion of row-security policy when processing a security barrier subquery created by a row-security policy, as it is desirable to respect the row-security policy of *other* tables that get referenced in the expanded row-security qual. If we just record the relid of the relation a subquery was expanded from and avoid expanding that inside the generated subquery we prevent simple linear recursion, but not mutual recursion between two or more rels with row-security policy. KaiGai's row-security patch handles this because it does its own recursive expansion, so (like the rewriter) it can keep a breadcrumb trail and detect when it is repeating a path. That's not so practical when row-security code tags RTEs with policy, then updatable s.b. views goes and expands them. So. Options. 1.Come up with a reasonable way to track recursive row-security expansion, detect infinite recursion, and bail out. Likely to involve a new global structure with planner/optimizer lifetime that gets checked and maintained by apply_row_security_rangetbl. 2.Solve the linear recursion case by storing a relid that should not get expanded for security quals when processing a subquery. Say don't do that, expect stack exceeded crashes if you do for the mutual recursion case. Seems user hostile, incomplete, and likely to break people's systems when they really don't expect it. 3.Disregard row-security policy on referenced tables when expanding row-security qualifiers. There's precendent here in foreign keys, which ignore row-security policy, but I don't think this is at all appealing. 4.Magic? My first thought is to add a boolean flag to RangeTblEntry (similar to the inh flag) to say whether RLS expansion is requested for that RTE. Then set it to false on each RTE as you add new RLS quals to it's securityQuals. In addition, when adding RLS quals to an RTE, I think they should be fully and recursively expanded immediately, before setting the new flag to false and moving on --- think recursively calling the rewriter to expand view references in the new RLS qual, and expand_security_qual() to expand any additional RLS quals in the securityQuals list --- with loop detection there. I'm not pretending that's going to be easy, but there are a couple of existing pieces of code to borrow ideas from. Doing it this way should make it possible to do the loop detection without any global structures. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
